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

feat(location)!: latitude/longitude returns number #1064

Merged
merged 11 commits into from
Oct 16, 2022

Conversation

xDivisionByZerox
Copy link
Member

This PR changes to return type of address.latitude() and address.longitude() from string to number.

Related to #1058.

This is a breaking change! Do not merge until the next major release.

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent do NOT merge yet Do not merge this PR into the target branch yet 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 labels Jun 12, 2022
@xDivisionByZerox xDivisionByZerox added this to the vFuture milestone Jun 12, 2022
@xDivisionByZerox xDivisionByZerox self-assigned this Jun 12, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner June 12, 2022 10:06
@codecov
Copy link

codecov bot commented Jun 12, 2022

Codecov Report

Merging #1064 (dc05add) into next (20f2236) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1064      +/-   ##
==========================================
- Coverage   99.63%   99.63%   -0.01%     
==========================================
  Files        2164     2164              
  Lines      236854   236852       -2     
  Branches     1004     1002       -2     
==========================================
- Hits       235991   235977      -14     
- Misses        842      854      +12     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/address/index.ts 99.77% <100.00%> (-0.01%) ⬇️
src/modules/internet/user-agent.ts 83.06% <0.00%> (-3.18%) ⬇️

ST-DDT
ST-DDT previously approved these changes Jun 12, 2022
src/modules/address/index.ts Outdated Show resolved Hide resolved
src/modules/address/index.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Jun 13, 2022

Considering the other PR, we should consider whether we want to introduce a format parameter instead.

@xDivisionByZerox
Copy link
Member Author

Considering the other PR, we should consider whether we want to introduce a format parameter instead.

I really dislike the idea. For me, it doesn't make sense that coordinates would be strings. But we (as a library) should allow stringified strings as input parameters due to the nature of JS and enhancing the dx when using Faker.

@ST-DDT
Copy link
Member

ST-DDT commented Jun 13, 2022

I really dislike the idea. For me, it doesn't make sense that coordinates would be strings. But we (as a library) should allow stringified strings as input parameters due to the nature of JS and enhancing the dx when using Faker.

Further discussion regarding that topic please in #1066 or a separate issue/discussion.

@xDivisionByZerox xDivisionByZerox force-pushed the refactor/address/lat-long-return-type branch from f094a7f to 581bcd2 Compare July 1, 2022 14:38
ST-DDT
ST-DDT previously approved these changes Jul 2, 2022
@xDivisionByZerox xDivisionByZerox added the m: location Something is referring to the location module label Jul 28, 2022
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Sep 23, 2022
@ST-DDT ST-DDT removed the do NOT merge yet Do not merge this PR into the target branch yet label Oct 12, 2022
ST-DDT
ST-DDT previously approved these changes Oct 13, 2022
@xDivisionByZerox xDivisionByZerox removed the needs rebase There is a merge conflict label Oct 13, 2022
@Shinigami92 Shinigami92 changed the title refactor(address)!: latitude/longitude return type refactor(location)!: change latitude/longitude to return number Oct 13, 2022
@Shinigami92
Copy link
Member

I eagerly put the PR title to scope location as we will solve

in v8.0 and this will then be grouped together

Shinigami92
Shinigami92 previously approved these changes Oct 13, 2022
src/modules/address/index.ts Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Oct 13, 2022
import-brain
import-brain previously approved these changes Oct 14, 2022
@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label Oct 14, 2022
@Shinigami92 Shinigami92 changed the title refactor(location)!: change latitude/longitude to return number feat(location)!: latitude/longitude returns number Oct 14, 2022
@Shinigami92
Copy link
Member

@xDivisionByZerox needs rebase

@Shinigami92 Shinigami92 dismissed stale reviews from import-brain, ST-DDT, and themself via eba83ad October 16, 2022 20:57
@Shinigami92
Copy link
Member

I tried to solve it, via the GitHub UI
Will fix it...

@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Oct 16, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) October 16, 2022 21:25
@Shinigami92 Shinigami92 merged commit dac6be3 into next Oct 16, 2022
@Shinigami92 Shinigami92 deleted the refactor/address/lat-long-return-type branch October 16, 2022 21:26
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
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants