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): add city_name to city_patterns #2104

Merged
merged 4 commits into from
Apr 30, 2023

Conversation

xDivisionByZerox
Copy link
Member

@xDivisionByZerox xDivisionByZerox commented Apr 28, 2023

Part of #2021.

Comment REF

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs c: locale Permutes locale definitions labels Apr 28, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team April 28, 2023 10:45
@xDivisionByZerox xDivisionByZerox self-assigned this Apr 28, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner April 28, 2023 10:45
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #2104 (f33437b) into next (bfd877f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f33437b differs from pull request most recent head 2340cdc. Consider uploading reports for the commit 2340cdc to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2104   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2567     2567           
  Lines      243352   243357    +5     
  Branches     1249     1252    +3     
=======================================
+ Hits       242361   242381   +20     
+ Misses        964      949   -15     
  Partials       27       27           
Impacted Files Coverage Δ
src/locales/de/location/city_pattern.ts 100.00% <100.00%> (ø)
src/locales/en_CA/location/city_pattern.ts 100.00% <100.00%> (ø)
src/locales/it/location/city_pattern.ts 100.00% <100.00%> (ø)
src/locales/ka_GE/location/city_pattern.ts 100.00% <100.00%> (ø)
src/locales/ur/location/city_pattern.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

Shinigami92
Shinigami92 previously approved these changes Apr 28, 2023
src/locales/nb_NO/location/city_pattern.ts Outdated Show resolved Hide resolved
src/locales/ko/location/city_pattern.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

What is the state of this PR? should this still be merged for v8.0?
Is it blocking?

@matthewmayer
Copy link
Contributor

What is the state of this PR? should this still be merged for v8.0? Is it blocking?

i think the no_NB and ko changes should be reverted and then the rest can be merged. Then those two languages can be better handled in another PR.

@xDivisionByZerox
Copy link
Member Author

I reverted the changes for ko and nb_NO

@ST-DDT ST-DDT merged commit b72d52e into next Apr 30, 2023
@ST-DDT ST-DDT deleted the locale/add-city-name-to-city-patterns branch April 30, 2023 11:28
@ST-DDT ST-DDT linked an issue May 1, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Merge faker.location.city() and faker.location.cityName()
4 participants