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

refactor(locale)!: use snake case for all locale data #2910

Merged
merged 5 commits into from
May 25, 2024

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented May 18, 2024

Standardize file naming convention for the renaming locale files.

old replacement
faker.definitions.science.chemicalElement faker.definitions.science.chemical_element
faker.definitions.system.directoryPaths faker.definitions.system.directory_paths
faker.definitions.system.mimeTypes faker.definitions.system.mime_types

This is an attempt an fixing some TODOs in our sources.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs c: locale Permutes locale definitions breaking change Cannot be merged when next version is not a major release labels May 18, 2024
@ST-DDT ST-DDT added this to the v9.0 milestone May 18, 2024
@ST-DDT ST-DDT requested review from a team May 18, 2024 12:19
@ST-DDT ST-DDT self-assigned this May 18, 2024
Copy link

netlify bot commented May 18, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 425f1c7
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/664f69cdbae75f00089e2dbc
😎 Deploy Preview https://deploy-preview-2910.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented May 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (b97984c) to head (425f1c7).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2910   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files        2977     2977           
  Lines      215421   215421           
  Branches      948      951    +3     
=======================================
+ Hits       215327   215333    +6     
+ Misses         94       88    -6     
Files Coverage Δ
src/locales/base/system/directory_paths.ts 100.00% <ø> (ø)
src/locales/base/system/index.ts 100.00% <100.00%> (ø)
src/locales/base/system/mime_types.ts 100.00% <ø> (ø)
src/locales/en/science/chemical_element.ts 100.00% <ø> (ø)
src/locales/en/science/index.ts 100.00% <100.00%> (ø)
src/locales/eo/science/chemical_element.ts 100.00% <ø> (ø)
src/locales/eo/science/index.ts 100.00% <100.00%> (ø)
src/locales/nb_NO/science/chemical_element.ts 100.00% <ø> (ø)
src/locales/nb_NO/science/index.ts 100.00% <100.00%> (ø)
src/locales/pl/science/chemical_element.ts 100.00% <ø> (ø)
... and 5 more

... and 1 file with indirect coverage changes

src/definitions/system.ts Show resolved Hide resolved
src/definitions/system.ts Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member

Why do we couple file anme and property key anyway? The propblem I have right now is that we have locale entries which might be lists of objects which intern have property keys as well. These are not considered in the PR. For example, look at en/airline/airline which is types as a list of Airline. It has the property iataCode, which is camelCase. So what I can do is faker.helpers.fake('airline.airline.iataCode') which contradictes the statement from the migration guide:

All our locale data now use the snake_case naming scheme.

@ST-DDT
Copy link
Member Author

ST-DDT commented May 18, 2024

Why do we couple file name and property key anyway?

Because the index files that merge them are generated.

We could transform the property keys in between, but it would be a massive breaking change. Sure we could do that, but we should talk about it first.

@ST-DDT
Copy link
Member Author

ST-DDT commented May 18, 2024

If you are just concerned about the wording, I can change it to refer to locale categories and entries specifically.

@xDivisionByZerox
Copy link
Member

If you are just concerned about the wording, I can change it to refer to locale categories and entries specifically.

Thats propably it for me. Alternativly we could actually make all properties snake_case. The definition properties which are not "correctly" cased right now (ignoring the ones from this PR) are:

  • airline.airline.iataCode
  • airline.airport.iataCode
  • internet.http_status_code.clientError
  • internet.http_status_code.serverError
  • science.chemical_element.atomicNumber

@ST-DDT ST-DDT requested review from xDivisionByZerox and a team May 20, 2024 20:00
@ST-DDT ST-DDT requested review from matthewmayer and a team May 23, 2024 16:11
@ST-DDT ST-DDT merged commit 558b959 into next May 25, 2024
20 checks passed
@ST-DDT ST-DDT deleted the refactor/locale/snake-case branch May 25, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: locale Permutes locale definitions c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants