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

fix(locale): remove unrealistic fictional city patterns in ne, ro, tr #2120

Merged
merged 3 commits into from
May 3, 2023

Conversation

matthewmayer
Copy link
Contributor

Removes fictional patterns in three locales where location.city_name is available but there are no native city prefixes/suffixes available.

Avoids weird combinations of ne/ro/tr names with English prefixes/suffixes like

West Vladcester
Bökeshire
Manishacester

example:
West Vladcester
Bökeshire
Manishacester
@matthewmayer matthewmayer requested a review from a team as a code owner May 2, 2023 13:03
@matthewmayer matthewmayer self-assigned this May 2, 2023
@matthewmayer matthewmayer added m: location Something is referring to the location module c: locale Permutes locale definitions p: 1-normal Nothing urgent labels May 2, 2023
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #2120 (99d4f5c) into next (7f9e9df) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 99d4f5c differs from pull request most recent head 2650cec. Consider uploading reports for the commit 2650cec to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2120      +/-   ##
==========================================
- Coverage   99.60%   99.60%   -0.01%     
==========================================
  Files        2605     2605              
  Lines      244968   244926      -42     
  Branches     1256     1254       -2     
==========================================
- Hits       243998   243949      -49     
- Misses        943      950       +7     
  Partials       27       27              
Impacted Files Coverage Δ
src/locales/el/location/city_name.ts 100.00% <ø> (ø)
src/locales/el/location/city_pattern.ts 100.00% <100.00%> (ø)
src/locales/el/location/index.ts 100.00% <100.00%> (ø)
src/locales/ne/location/city_pattern.ts 100.00% <100.00%> (ø)
src/locales/ro/location/city_pattern.ts 100.00% <100.00%> (ø)
src/locales/sk/location/city_prefix.ts 100.00% <100.00%> (ø)
src/locales/sk/location/city_suffix.ts 100.00% <100.00%> (ø)
src/locales/tr/location/city_pattern.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@matthewmayer matthewmayer changed the title fix(locale): remove unrealistic fictional patterns in ne, ro, tr fix(locale): remove unrealistic fictional city patterns in ne, ro, tr May 2, 2023
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

@ST-DDT ST-DDT requested review from a team May 2, 2023 14:31
@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented May 2, 2023

Removes fictional patterns in three locales where location.city_name is available but there are no native city prefixes/suffixes available.

Do those prefixes/suffixes simply not exist in our dataset or is the concept itself non existing in the locale? Do you have any info on that?

If that should be the case, please export null from the according data files. Otherwise, I think we should fill in those data instead of removing variants.

@matthewmayer
Copy link
Contributor Author

I'd imagine most languages would have certain prefixes or suffixes which might appear commonly in city names

If someone who knows more about the language comes by later they could add back some new fictional patterns with appropriate suffixes and prefixes.

But for now, just using the real city names seems superior.

@ST-DDT ST-DDT enabled auto-merge (squash) May 3, 2023 16:50
@ST-DDT ST-DDT merged commit d9ec87e into faker-js:next May 3, 2023
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: location Something is referring to the location module p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants