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 types #2550

Merged
merged 18 commits into from Dec 25, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Nov 15, 2023

Fixes #1850
Supersedes #2381

This PR adds a new intermediate method faker.helpers.eval that replaces the entire fake pattern placeholder resolution that was previously located inside the fake method.

I extracted the resolution part in its own method because it would otherwise get too long/complex to read.

faker.helpers.fake("{{person.firstName}}) -> faker.helpers.eval("person.firstName") -> faker.person.firstName()

The eval function is now also cable to resolve expressions that contain more than the previous two parts.

This is needed because of methods such as faker.airline.airline() that return complex objects.

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent m: helpers Something is referring to the helpers module labels Nov 15, 2023
@ST-DDT ST-DDT added this to the v8.x milestone Nov 15, 2023
@ST-DDT ST-DDT requested review from a team November 15, 2023 13:55
@ST-DDT ST-DDT self-assigned this Nov 15, 2023
@ST-DDT ST-DDT linked an issue Nov 15, 2023 that may be closed by this pull request
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (c209030) 99.57% compared to head (13669a8) 99.57%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2550      +/-   ##
==========================================
- Coverage   99.57%   99.57%   -0.01%     
==========================================
  Files        2806     2807       +1     
  Lines      250195   250374     +179     
  Branches     1104     1135      +31     
==========================================
+ Hits       249130   249300     +170     
- Misses       1037     1046       +9     
  Partials       28       28              
Files Coverage Δ
src/modules/helpers/index.ts 98.98% <100.00%> (-0.04%) ⬇️
src/modules/helpers/eval.ts 96.94% <96.94%> (ø)

... and 2 files with indirect coverage changes

src/modules/helpers/eval.ts Outdated Show resolved Hide resolved
test/modules/helpers.spec.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

Does eval need to be public? It seems it could be an internal helper method. I don't think library users would normally have any need to use it?

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 15, 2023

Does eval need to be public?

No, it doesnt need to be public. But it covers some usecases, that fake cannot because it is bound to be a string, whereas eval can return anything.

The eval function is useful for tools like:

That wish to generate any data based on a schema definition, a jsdoc annotation, or a @annotation.

I'm fine with exposing it in a separate PR.

@matthewmayer
Copy link
Contributor

I think there might be logic in making it private first just because then we don't need to worry about any breaking changes, or publicly documenting it. Once we are clear it is stable and useful for third parties it could be exposed.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 15, 2023

I agree with the publicly documenting part, but the expectations (except for the entrypoint parameters) are already defined by fake because it the actual implementation fake uses to resolve its stuff. IMO whether this is private or public is a small detail that can be adjusted anytime (before merge). What about the rest of the implementation?

@matthewmayer
Copy link
Contributor

Well if it's public that cannot be changed to private at "any time" only on a semver major release 😀

@xDivisionByZerox
Copy link
Member

I have to agree with @matthewmayer here. While there might be a use case for this method, I think it will be a really niche one. Even if not, we don't lose anything by making it internal for now. While, as already said, we might have some maintainable burdens when making it public first.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 15, 2023

Sorry, there might have been some miscommunication here.
My issue was never whether the function was public or not, I only wanted some feedback on the actual implementation (and whether I should scrap the entire concept or whether it is worth continuing on).

I hid away the method now, so it does not cause any further confusion during reviews.

test/modules/helpers.spec.ts Show resolved Hide resolved
test/modules/helpers.spec.ts Outdated Show resolved Hide resolved
test/modules/helpers-eval.spec.ts Show resolved Hide resolved
src/modules/helpers/eval.ts Outdated Show resolved Hide resolved
src/modules/helpers/eval.ts Outdated Show resolved Hide resolved
src/modules/helpers/eval.ts Show resolved Hide resolved
src/modules/helpers/eval.ts Show resolved Hide resolved
src/modules/helpers/eval.ts Outdated Show resolved Hide resolved
src/modules/helpers/eval.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@matthewmayer matthewmayer left a comment

Choose a reason for hiding this comment

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

faker.helpers.fake("{{person.first_name()}}") used to work but now throws. Maybe add a test to explicitly check for this?

In general due to subtle behavior changes like this i feel this should be considered breaking. I can imagine there are example in the wild of faker.helpers.fake("{{person.first_name()}}") instead of the correct faker.helpers.fake("{{person.firstName()}}") which will break.

@xDivisionByZerox
Copy link
Member

To not introduce breaking changes, could we ignore parenthesis for now?

@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 11, 2023

Done

@xDivisionByZerox xDivisionByZerox merged commit 24482a3 into next Dec 25, 2023
20 checks passed
@xDivisionByZerox xDivisionByZerox deleted the feat/helpers/eval-and-nested-patterns branch December 25, 2023 00:51
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 s: accepted Accepted feature / Confirmed bug
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