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(location)!: rename definition city to city_pattern #2094

Merged

Conversation

xDivisionByZerox
Copy link
Member

Fixes #2021, #1471.

@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 breaking change Cannot be merged when next version is not a major release m: location Something is referring to the location module labels Apr 25, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team April 25, 2023 14:34
@xDivisionByZerox xDivisionByZerox self-assigned this Apr 25, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner April 25, 2023 14:34
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #2094 (001202b) into next (5ad55a5) will increase coverage by 0.00%.
The diff coverage is 99.16%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2094   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2567     2567           
  Lines      243363   243365    +2     
  Branches     1249     1253    +4     
=======================================
+ Hits       242373   242387   +14     
+ Misses        963      951   -12     
  Partials       27       27           
Impacted Files Coverage Δ
src/definitions/location.ts 0.00% <0.00%> (ø)
src/locales/af_ZA/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/ar/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/az/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/cs_CZ/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/de/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/de_AT/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/de_CH/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/dv/location/city_pattern.ts 100.00% <ø> (ø)
src/locales/el/location/city_pattern.ts 100.00% <ø> (ø)
... and 108 more

... and 1 file with indirect coverage changes

@matthewmayer
Copy link
Contributor

this doesn't actually fix #2021 though right? Because the two methods still return different data.

@Shinigami92
Copy link
Member

this doesn't actually fix #2021 though right? Because the two methods still return different data.

We will start without a generated/fictional parameter and will add that back when users request them. (This avoids breaking the API when removing the parameter if unneeded)

@matthewmayer
Copy link
Contributor

But i mean even after this PR is merged, say

fakerEN.location.city() and fakerEN.location.cityName()
fakerDE.location.city() and fakerDE.location.cityName()

do not behave the same, so there would need to be another PR before #2021 is considered fixed?

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Apr 25, 2023

But i mean even after this PR is merged, say

fakerEN.location.city() and fakerEN.location.cityName() fakerDE.location.city() and fakerDE.location.cityName()

do not behave the same, so there would need to be another PR before #2021 is considered fixed?

No, because location.city() already returns a superset of location.cityName().
I have explained this to you this in #2070 (comment)

@matthewmayer
Copy link
Contributor

ok then maybe #2021 should be renamed? Because the methods are not being merged.

@matthewmayer
Copy link
Contributor

in what way does location.city() return a superset of location.cityName()?

fakerDE.location.cityName() can return Düsseldorf but fakerDE.location.city() will never return Düsseldorf

@xDivisionByZerox
Copy link
Member Author

in what way does location.city() return a superset of location.cityName()?

fakerDE.location.cityName() can return Düsseldorf but fakerDE.location.city() will never return Düsseldorf

In that case, that's an issue with the fakerDE instance. Not the method implementation.

@matthewmayer
Copy link
Contributor

Right so these locales will need fixing in a seperate PR if we want city() to also return their real city names

[
'de', 'dv', 'en',
'en_CA', 'it', 'ka_GE',
'ko', 'nb_NO', 'ne',
'ro', 'tr', 'uk',
'ur'
]

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Apr 25, 2023

The following locales are in fact able to return real city names:

@Shinigami92 Shinigami92 merged commit 8cd1965 into next Apr 27, 2023
@Shinigami92 Shinigami92 deleted the refactor/location/rename-city-definition-to-city-patterns branch April 27, 2023 15:33
matthewmayer added a commit to matthewmayer/faker that referenced this pull request May 13, 2023
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: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs 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.

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