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: dynamic definitions tree #822

Merged
merged 4 commits into from Apr 19, 2022
Merged

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Apr 9, 2022

Instead of making only our definitions/locale data accessible via faker.definitions, this PR allows devs to access any data, that are added to the locale itself.

Or in other words:
Previously faker.definitions.business returned undefined even though faker.locales.en.buisness is defined.

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent labels Apr 9, 2022
@ST-DDT ST-DDT added this to the v6.2 - New small features milestone Apr 9, 2022
@ST-DDT ST-DDT self-assigned this Apr 9, 2022
@ST-DDT ST-DDT requested a review from a team as a code owner April 9, 2022 14:02
@codecov
Copy link

codecov bot commented Apr 9, 2022

Codecov Report

Merging #822 (0cd2439) into main (d94159f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 0cd2439 differs from pull request most recent head 594b216. Consider uploading reports for the commit 594b216 to get more accurate results

@@           Coverage Diff           @@
##             main     #822   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1921     1921           
  Lines      179170   179183   +13     
  Branches      895      898    +3     
=======================================
+ Hits       177993   178006   +13     
  Misses       1121     1121           
  Partials       56       56           
Impacted Files Coverage Δ
src/definitions/definitions.ts 100.00% <100.00%> (ø)
src/faker.ts 100.00% <100.00%> (ø)

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 9, 2022

Partially addresses #823 .

@ST-DDT ST-DDT requested a review from a team April 10, 2022 18:13
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

To really understand what's going on in initDefinitions I would like to have a screenshare after my vacation with you, where I go through the code and you explain what I don't understand 🙂

src/faker.ts Outdated Show resolved Hide resolved
src/faker.ts Outdated Show resolved Hide resolved
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Tested it out locally with the new tests but without the changes in src
To me this is not a feature but a bugfix!

@ST-DDT ST-DDT requested a review from a team April 12, 2022 12:26
@Shinigami92 Shinigami92 merged commit 069f4d1 into main Apr 19, 2022
@Shinigami92 Shinigami92 deleted the feature/dynamic-definition-tree branch April 19, 2022 18:20
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 p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants