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): deprecate cityName #2070

Merged
merged 7 commits into from Apr 21, 2023

Conversation

xDivisionByZerox
Copy link
Member

Part of #2021.

@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 m: location Something is referring to the location module deprecation A deprecation was made in the PR labels Apr 19, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team April 19, 2023 12:36
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner April 19, 2023 12:36
@xDivisionByZerox xDivisionByZerox self-assigned this Apr 19, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team April 19, 2023 12:36
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

❗ No coverage uploaded for pull request base (next@2a6f578). Click here to learn what that means.
The diff coverage is 100.00%.

❗ Current head eaa21fa differs from pull request most recent head 89c38cc. Consider uploading reports for the commit 89c38cc to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2070   +/-   ##
=======================================
  Coverage        ?   99.61%           
=======================================
  Files           ?     2535           
  Lines           ?   242211           
  Branches        ?     1293           
=======================================
  Hits            ?   241274           
  Misses          ?      910           
  Partials        ?       27           
Impacted Files Coverage Δ
src/modules/random/index.ts 100.00% <ø> (ø)
src/modules/location/index.ts 99.17% <100.00%> (ø)

@ST-DDT
Copy link
Member

ST-DDT commented Apr 19, 2023

The other PR #2051 also adjusts the locale data.

@xDivisionByZerox
Copy link
Member Author

The other PR #2051 also adjusts the locale data.

I know but wanted to prevent merge conflicts as well as breaking changes in THIS PR. I had a really hard time actually finding the deprecation itself in the other PR.

@matthewmayer
Copy link
Contributor

Do you plan to make city and cityName return the same data in v8.0? Or you just deprecate cityName in v8.0 and merge the locale data in v9?

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Apr 19, 2023

This is the same as #2051.
location.city already is a superset of location.cityName. As described in #2021 (comment) we simply deprecate the *Name implementation for now. We can think about adding a generated option parameter (to only get generated or real cities) in the future if the community requests.

@Shinigami92 Shinigami92 enabled auto-merge (squash) April 21, 2023 16:40
@Shinigami92 Shinigami92 merged commit 9161a06 into next Apr 21, 2023
15 checks passed
@Shinigami92 Shinigami92 deleted the refactor/location/deprecate-city-name branch April 21, 2023 16:45
@ST-DDT ST-DDT linked an issue Apr 26, 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: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs deprecation A deprecation was made in the PR 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