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(locale): add Dutch province abbreviations #2232

Merged
merged 5 commits into from
Jul 9, 2023
Merged

feat(locale): add Dutch province abbreviations #2232

merged 5 commits into from
Jul 9, 2023

Conversation

RobinvanderVliet
Copy link
Contributor

@RobinvanderVliet RobinvanderVliet requested a review from a team as a code owner July 1, 2023 16:59
@codecov
Copy link

codecov bot commented Jul 1, 2023

Codecov Report

Merging #2232 (7c4d7a3) into next (5a48282) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2232      +/-   ##
==========================================
- Coverage   99.60%   99.60%   -0.01%     
==========================================
  Files        2639     2640       +1     
  Lines      245741   245757      +16     
  Branches     1158     1156       -2     
==========================================
+ Hits       244771   244779       +8     
- Misses        943      951       +8     
  Partials       27       27              
Impacted Files Coverage Δ
src/locales/nl/location/index.ts 100.00% <100.00%> (ø)
src/locales/nl/location/state.ts 100.00% <100.00%> (ø)
src/locales/nl/location/state_abbr.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

While I already approve, it would be nice if you could already sort the values alphabetically (a-z). We plan on
enforcing this via the locale generation script in the future. So if you could already do this we could reduce the git diff in the future :)

@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug c: locale Permutes locale definitions labels Jul 1, 2023
@xDivisionByZerox xDivisionByZerox added the m: location Something is referring to the location module label Jul 1, 2023
@RobinvanderVliet
Copy link
Contributor Author

While I already approve, it would be nice if you could already sort the values alphabetically (a-z). We plan on enforcing this via the locale generation script in the future. So if you could already do this we could reduce the git diff in the future :)

I intentionally kept them in the same order as state.ts, because I thought the two files were connected in some way.

If that is not needed, I can order them alphabetically.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 2, 2023

I intentionally kept them in the same order as state.ts, because I thought the two files were connected in some way.

If that is not needed, I can order them alphabetically.

No, the files aren't connected with each other, so please sort this one alphabetically.
We keep connected data in the same file so they can be changed in one step and dont risk them diverging from each other. See also our airline.airline data.

@RobinvanderVliet
Copy link
Contributor Author

I sorted them in this pull request and also in #2233.

@matthewmayer
Copy link
Contributor

@RobinvanderVliet don't worry about merging next into your PR branches, this will be done automatically before the feature is squash merged. You only need to bother if there are conflicts.

@ST-DDT ST-DDT changed the title feat(locale): Add Dutch province abbreviations feat(locale): add Dutch province abbreviations Jul 9, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) July 9, 2023 08:11
@ST-DDT ST-DDT merged commit 4a15bd0 into faker-js:next Jul 9, 2023
@RobinvanderVliet RobinvanderVliet deleted the add-dutch-province-abbreviations branch July 9, 2023 21:19
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 c: locale Permutes locale definitions m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants