Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(helpers): add support for complex intermediate return types in fake method #2381

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Sep 9, 2023

Fixes #1850

I started to add support for complex intermediate return types in the fake method of the helpers module as described in #1850.

Currently, it is a work in progress. I would like to get feedback from you.

I wrote a few tests and will add more in the next days, for example for locale data references.

@ghost ghost self-requested a review as a code owner September 9, 2023 11:47
@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Merging #2381 (acccce2) into next (c158b38) will decrease coverage by 0.01%.
Report is 31 commits behind head on next.
The diff coverage is 100.00%.

❗ Current head acccce2 differs from pull request most recent head bc9e533. Consider uploading reports for the commit bc9e533 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2381      +/-   ##
==========================================
- Coverage   99.60%   99.60%   -0.01%     
==========================================
  Files        2660     2786     +126     
  Lines      246345   252572    +6227     
  Branches     1086     1089       +3     
==========================================
+ Hits       245375   251577    +6202     
- Misses        943      968      +25     
  Partials       27       27              
Files Changed Coverage Δ
src/locales/en/finance/credit_card/index.ts 100.00% <ø> (ø)
src/locale/eo.ts 100.00% <100.00%> (ø)
src/locale/index.ts 100.00% <100.00%> (ø)
src/locale/yo_NG.ts 100.00% <100.00%> (ø)
src/locales/da/commerce/department.ts 100.00% <100.00%> (ø)
src/locales/da/commerce/index.ts 100.00% <100.00%> (ø)
src/locales/da/commerce/product_description.ts 100.00% <100.00%> (ø)
src/locales/da/commerce/product_name.ts 100.00% <100.00%> (ø)
src/locales/da/company/adjective.ts 100.00% <100.00%> (ø)
src/locales/da/company/buzz_adjective.ts 100.00% <100.00%> (ø)
... and 137 more

... and 4 files with indirect coverage changes

@xDivisionByZerox xDivisionByZerox assigned ghost Sep 9, 2023
@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature p: 1-normal Nothing urgent m: helpers Something is referring to the helpers module labels Sep 9, 2023
src/modules/lorem/index.ts Outdated Show resolved Hide resolved
});

it('should return undefined if a method returns a complex object but the property is undefined', () => {
expect(faker.helpers.fake('{{airline.airport.code}}')).toBe(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently if a property is unknown on we receive an error:

Invalid module method or definition: person.abc
- faker.person.abc is not a function
- faker.definitions.person.abc is not an array

I would prefer if we could throw this error here as well and add a case for faker.person is a function but does not return an object with the property abc. I'll gladly take suggestions on the phrasing.

);
});

it('should be able to pass multiple dynamic templates with complex objects', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this test actually adds any value since we already have a test for an array argument:

it('should be able to pass multiple dynamic templates', () => {
expect(faker.definitions.location.city_name).toContain(
faker.helpers.fake([
'{{location.city_name}}',
'{{location.cityName}}',
])
);
});

@xDivisionByZerox xDivisionByZerox requested review from a team September 9, 2023 19:47
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a test that checks that if the fn didnt return a complex object and the property was requested that then an error is thrown.

Also I'm not sure about the current implementation.
E.g. Because it would break for {{airline.airport().iataCode}}
and it no longer supports it for 4 or deeper properties, but I'm not sure it has to.

@matthewmayer
Copy link
Contributor

Which of these do we expect to work

A {{airline.airport().iataCode}}

B {{airline.airport.iataCode}}

C {{airline.airport.iataCode()}}

@ST-DDT
Copy link
Member

ST-DDT commented Sep 10, 2023

Which of these do we expect to work

A {{airline.airport().iataCode}}

B {{airline.airport.iataCode}}

C {{airline.airport.iataCode()}}

I would expect A and B to work.

@ST-DDT
Copy link
Member

ST-DDT commented Nov 21, 2023

Thanks for your contribution ❤️, we used it to create a new PR for this feature.


Superseded by #2550

@ST-DDT ST-DDT closed this Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for complex intermediate return types in fake method
3 participants