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.

Sign up for GitHub

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

Jump to bottom

refactor(location)!: rename definition street to street_pattern #2051

Merged
merged 13 commits into from Apr 21, 2023
Merged

refactor(location)!: rename definition street to street_pattern #2051

merged 13 commits into from Apr 21, 2023

Conversation

Shinigami92
Copy link
Member

close #2020

@Shinigami92 Shinigami92 added 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 labels Apr 15, 2023
@Shinigami92 Shinigami92 self-assigned this Apr 15, 2023
@Shinigami92
Copy link
Member Author

Shinigami92 commented Apr 15, 2023 •
edited

  1. Is this a breaking change? Do we need to add the ! in PR title?
  2. How should I name this PR/changelog entry?
  3. What do we need to do for documentation?

@codecov
Copy link

codecov bot commented Apr 15, 2023 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (next@14a033a). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head ad6a9b1 differs from pull request most recent head 2ad2d86. Consider uploading reports for the commit 2ad2d86 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             next    #2051   +/-   ##
=======================================
  Coverage        ?   99.61%           
=======================================
  Files           ?     2536           
  Lines           ?   242236           
  Branches        ?     1298           
=======================================
  Hits            ?   241307           
  Misses          ?      902           
  Partials        ?       27

@xDivisionByZerox xDivisionByZerox mentioned this pull request Apr 15, 2023
Closed
@Shinigami92 Shinigami92 changed the title refactor(location): street refactor(location)!: deprecate streetName and internally rename street to street_pattern definition Apr 16, 2023
@Shinigami92
Copy link
Member Author

Shinigami92 commented Apr 16, 2023 •
edited

  1. Is this a breaking change? Do we need to add the ! in PR title?

We decided yes, just to have this at the top in the changelog

  1. How should I name this PR/changelog entry?

Still unsure how to name this
Especially the internal stuff is not really relevant

  1. What do we need to do for documentation?

No response yet

@Shinigami92 Shinigami92 marked this pull request as ready for review April 16, 2023 10:22
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 16, 2023 10:22
matthewmayer
matthewmayer previously requested changes Apr 16, 2023
View reviewed changes
src/definitions/location.ts Show resolved Hide resolved
ST-DDT
ST-DDT reviewed Apr 16, 2023
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment •
edited

Choose a reason for hiding this comment

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

If I checked correctly the following locales have street_names, but no pattern using it.

  • az
  • es_MX
  • he
  • ka_GE
  • ko
  • lv
  • nb_NO
  • ro
  • ru
  • sv
  • uk

I will create a new issue for this: #2062

src/modules/location/index.ts Outdated Show resolved Hide resolved
src/locales/th/location/street_pattern.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Apr 16, 2023
@ST-DDT ST-DDT mentioned this pull request Apr 16, 2023
Open
ST-DDT
ST-DDT previously approved these changes Apr 16, 2023
View reviewed changes
@ST-DDT ST-DDT requested review from a team April 16, 2023 13:27
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Apr 16, 2023
@matthewmayer
Copy link
Contributor

why is this marked as breaking in the PR desc?

@ST-DDT
Copy link
Member

ST-DDT commented Apr 16, 2023

Because it breaks your code if you use fake("{{location.street}}").

@matthewmayer
Copy link
Contributor

Won't {{location.street}} grab the method not the definitions?

@Shinigami92
Copy link
Member Author

Won't {{location.street}} grab the method not the definitions?

I expect yes

@Shinigami92 Shinigami92 dismissed matthewmayer’s stale review April 18, 2023 18:00

please review or test what you need

@matthewmayer
Copy link
Contributor

Looks ok but shouldn't be marked breaking if it isn't.

@matthewmayer
Copy link
Contributor

On https://next.fakerjs.dev/guide/upgrading.html#faker-address-changed-to-faker-location perhaps the row for streetName should be updated to reflect that if you are upgrading from faker.address.streetName then you should migrate straight to faker.location.street instead of faker.location.streetName?

@ST-DDT ST-DDT mentioned this pull request Apr 19, 2023
Merged
@ST-DDT
Copy link
Member

ST-DDT commented Apr 19, 2023

Should we split this PR into two?
Both changes are indepent from each other.
#2070 (comment)

@xDivisionByZerox
Copy link
Member

Should we split this PR into two? Both changes are indepent from each other. #2070 (comment)

Would be a welcome change but not required since we already reviewed it IMO

@ST-DDT
Copy link
Member

ST-DDT commented Apr 19, 2023

Would be a welcome change but not required since we already reviewed it IMO

The migration guide needs updating anyway so it doesnt really matter IMO.

@ST-DDT ST-DDT dismissed their stale review April 19, 2023 15:44

migration guide needs updating

@Shinigami92 Shinigami92 mentioned this pull request Apr 20, 2023
Merged
@Shinigami92
Copy link
Member Author

#2071 should be merged first, so this can be rebased

@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label Apr 20, 2023
@Shinigami92 Shinigami92 removed the s: on hold Blocked by something or frozen to avoid conflicts label Apr 21, 2023
@Shinigami92 Shinigami92 changed the title refactor(location)!: deprecate streetName and internally rename street to street_pattern definition refactor(location)!: rename definition street to street_pattern Apr 21, 2023
@Shinigami92 Shinigami92 requested a review from ST-DDT April 21, 2023 16:48
@ST-DDT ST-DDT requested review from a team April 21, 2023 17:01
@Shinigami92 Shinigami92 enabled auto-merge (squash) April 21, 2023 21:25
@Shinigami92 Shinigami92 merged commit 3529ef7 into next Apr 21, 2023
15 checks passed
@Shinigami92 Shinigami92 deleted the 2020-street branch May 1, 2023 08:30
@xDivisionByZerox xDivisionByZerox mentioned this pull request May 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

@import-brain import-brain import-brain approved these changes

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

@matthewmayer matthewmayer Awaiting requested review from matthewmayer

Assignees

@Shinigami92 Shinigami92

Labels
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
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.

Merge faker.location.street() and faker.location.streetName()
5 participants
@Shinigami92 @matthewmayer @ST-DDT @xDivisionByZerox @import-brain