Skip to content

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

Merged
Shinigami92 merged 13 commits into
nextfrom
2020-street
Apr 21, 2023
Merged

refactor(location)!: rename definition street to street_pattern#2051
Shinigami92 merged 13 commits into
nextfrom
2020-street

Conversation

@Shinigami92

Copy link
Copy Markdown
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

Shinigami92 commented Apr 15, 2023

Copy link
Copy Markdown
Member Author
  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

codecov Bot commented Apr 15, 2023

Copy link
Copy Markdown

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           
@Shinigami92 Shinigami92 changed the title refactor(location): street refactor(location)!: deprecate streetName and internally rename street to street_pattern definition Apr 16, 2023
@Shinigami92

Shinigami92 commented Apr 16, 2023

Copy link
Copy Markdown
Member Author
  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
Comment thread src/definitions/location.ts

@ST-DDT ST-DDT left a comment

Copy link
Copy Markdown
Member

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

Comment thread src/modules/location/index.ts Outdated
Comment thread src/locales/th/location/street_pattern.ts
@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Apr 16, 2023
ST-DDT
ST-DDT previously approved these changes Apr 16, 2023
@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
Copy Markdown
Contributor

why is this marked as breaking in the PR desc?

@ST-DDT

ST-DDT commented Apr 16, 2023

Copy link
Copy Markdown
Member

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

@matthewmayer

Copy link
Copy Markdown
Contributor

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

@Shinigami92

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@matthewmayer

Copy link
Copy Markdown
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 commented Apr 19, 2023

Copy link
Copy Markdown
Member

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

@xDivisionByZerox

Copy link
Copy Markdown
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

ST-DDT commented Apr 19, 2023

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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
@Shinigami92 Shinigami92 deleted the 2020-street branch May 1, 2023 08:30
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 m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

5 participants