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): remove fr_CH data which is identical to fr #2526

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Nov 4, 2023

Removed identical animal, color, commerce, date, finance, music, vehicle and word definitions which can use fallback from fr instead

Debatable if this should be considered breaking. Would only affect behavior if you were using a custom Faker instance with the fr_CH data but no fr fallback

@matthewmayer matthewmayer requested a review from a team as a code owner November 4, 2023 02:50
@matthewmayer matthewmayer marked this pull request as draft November 4, 2023 03:03
Removed identical animal, color, commerce, date, finance, music, vehicle and word definitions which can use fallback from fr instead
@matthewmayer matthewmayer marked this pull request as ready for review November 4, 2023 03:10
Copy link

codecov bot commented Nov 4, 2023

Codecov Report

Merging #2526 (8bfc804) into next (9daa744) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 8bfc804 differs from pull request most recent head 56ad953. Consider uploading reports for the commit 56ad953 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2526      +/-   ##
==========================================
- Coverage   99.58%   99.57%   -0.01%     
==========================================
  Files        2820     2777      -43     
  Lines      255037   249352    -5685     
  Branches     1080     1102      +22     
==========================================
- Hits       253982   248301    -5681     
+ Misses       1027     1023       -4     
  Partials       28       28              
Files Coverage Δ
src/locales/fr_CH/index.ts 100.00% <ø> (ø)

... and 28 files with indirect coverage changes

@matthewmayer matthewmayer self-assigned this Nov 4, 2023
@matthewmayer matthewmayer added p: 1-normal Nothing urgent c: locale Permutes locale definitions labels Nov 4, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Nov 4, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Nov 4, 2023

This PR can easily be reviewed with the following script executed on next:

diff -rs src/locales/fr_CH src/locales/fr | egrep '^Files .+ and .+ are identical$' | awk -F '(Files | and | are identical)' '{print $2}'

Although according to the script the following files are identical as well:

  • src/locales/fr_CH/location/building_number.ts
  • src/locales/fr_CH/location/city_pattern.ts
  • src/locales/fr_CH/location/country.ts
  • src/locales/fr_CH/location/direction.ts
  • src/locales/fr_CH/location/secondary_address.ts
  • src/locales/fr_CH/location/street_address.ts
  • src/locales/fr_CH/location/street_pattern.ts
  • src/locales/fr_CH/lorem/index.ts
  • src/locales/fr_CH/lorem/words.ts
  • src/locales/fr_CH/person/female_prefix.ts
  • src/locales/fr_CH/person/first_name.ts
  • src/locales/fr_CH/person/index.ts
  • src/locales/fr_CH/person/last_name_pattern.ts
  • src/locales/fr_CH/person/male_prefix.ts
  • src/locales/fr_CH/person/name.ts
  • src/locales/fr_CH/person/prefix.ts
  • src/locales/fr_CH/person/sex.ts
  • src/locales/fr_CH/person/title.ts
  • src/locales/fr_CH/phone_number/index.ts

to remove them append | xargs rm

@matthewmayer
Copy link
Contributor Author

I decided to only touch modules where the entire module was identical.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Nov 4, 2023
@xDivisionByZerox
Copy link
Member

A little reminder that the following contribution guideline should be respected by members and maintainers as well:

Adding new locale or updating existing one

[...]
Please only change files related to one module (e.g. person, location) whenever possible. This can simplify/speed up the review process. Additionally, it allows the maintainers to track PRs in a meaningful way by adding related labels.

@matthewmayer
Copy link
Contributor Author

I can split this if you want, but as this is only removals not additions i feel there's a lot less to review (sourcing, translations, etc)

@xDivisionByZerox
Copy link
Member

I totally understand that reasoning. Nevertheless, just imagine someone outside the core team providing a PR in that way because they "feel" like it. We would point to our contribution rules and asked them to split the PR. If not we would not need those rules in the first place.


All that being said, I think we can let that slip this time. You don't need split this PR - at least from my point of view.
As I said aboth: Just "A little reminder" 😄

@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Nov 5, 2023

This PR reduces the bundle size of the package by the following amounts:

Before (358572d) Current (8bfc804) Reduction Peercent
Package Size 2.4 MB 2.4 MB N/A
Unpacked Size 10.3 MB 10.1 MB 1,94%

I tryed this out of curiosity, but might that be interesting information for us on PRs that have the c: locale label? (sorry for the off topic question)

@ST-DDT
Copy link
Member

ST-DDT commented Nov 5, 2023

A little reminder that the following contribution guideline should be respected by members and maintainers as well:

Adding new locale or updating existing one

[...]
Please only change files related to one module (e.g. person, location) whenever possible. This can simplify/speed up the review process. Additionally, it allows the maintainers to track PRs in a meaningful way by adding related labels.

IMO that rule only applies to additions or changes that pertains the functional values within.
Removals or auto-transformed stuff doesn't have to be split into different PRs.
The rule is there to help us review new/external things. I assume you put this comment here due to #2265 (comment)
If you consider the split for those scripts too much (unnecessary) work, we can discuss that again.

@xDivisionByZerox xDivisionByZerox added m: animal Something is referring to the animal module m: color Something is referring to the color module m: commerce Something is referring to the commerce module m: date Something is referring to the date module m: finance Something is referring to the finance module m: music Something is referring to the music module m: vehicle Something is referring to the vehicle module m: word Something is referring to the word module labels Nov 5, 2023
@xDivisionByZerox xDivisionByZerox requested review from a team November 5, 2023 12:52
@ST-DDT
Copy link
Member

ST-DDT commented Nov 5, 2023

I decided to only touch modules where the entire module was identical.

What about the other identical files. Do you do them as a separate PR?

@matthewmayer
Copy link
Contributor Author

I decided to only touch modules where the entire module was identical.

What about the other identical files. Do you do them as a separate PR?

My feeling is they can be left for now. Realistically all fr_XX locales will use the same words for animals, dates, etc but will more likely use different patterns for names, locations etc. So even if two files are identical in fr_XX/location/YYY.ts currently, that's probably temporary - it probably makes it easier for future contributors if there's already a file they can directly edit rather than have to figure out the overrides.

@xDivisionByZerox xDivisionByZerox removed the s: needs decision Needs team/maintainer decision label Nov 6, 2023
@xDivisionByZerox xDivisionByZerox enabled auto-merge (squash) November 6, 2023 18:36
@xDivisionByZerox xDivisionByZerox merged commit fafcba4 into faker-js:next Nov 6, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions m: animal Something is referring to the animal module m: color Something is referring to the color module m: commerce Something is referring to the commerce module m: date Something is referring to the date module m: finance Something is referring to the finance module m: music Something is referring to the music module m: vehicle Something is referring to the vehicle module m: word Something is referring to the word module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants