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)!: nearbyGPSCoordinate returns number tuple #1061

Merged
merged 8 commits into from
Oct 14, 2022

Conversation

xDivisionByZerox
Copy link
Member

Discussion reference #1058.

Changes the return type of address.nearbyGPSCoordinate() from a string tuple to a number tuple.

This is a breaking change and should not be merged before the next major release.

@xDivisionByZerox xDivisionByZerox added 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 11, 2022
@xDivisionByZerox xDivisionByZerox added this to the vFuture milestone Jun 11, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team June 11, 2022 19:16
@xDivisionByZerox xDivisionByZerox self-assigned this Jun 11, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner June 11, 2022 19:16
@xDivisionByZerox xDivisionByZerox added the do NOT merge yet Do not merge this PR into the target branch yet label Jun 11, 2022
@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #1061 (2513123) into next (2c9622b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 2513123 differs from pull request most recent head 5ab3c8b. Consider uploading reports for the commit 5ab3c8b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1061      +/-   ##
==========================================
- Coverage   99.61%   99.60%   -0.01%     
==========================================
  Files        2166     2164       -2     
  Lines      237450   236816     -634     
  Branches     1040      999      -41     
==========================================
- Hits       236543   235888     -655     
- Misses        886      906      +20     
- Partials       21       22       +1     
Impacted Files Coverage Δ
src/modules/commerce/index.ts 100.00% <ø> (ø)
src/modules/helpers/index.ts 98.44% <ø> (-0.07%) ⬇️
src/modules/image/providers/lorempicsum.ts 93.85% <ø> (-1.00%) ⬇️
src/modules/image/providers/lorempixel.ts 91.30% <ø> (-0.65%) ⬇️
src/modules/image/providers/unsplash.ts 100.00% <ø> (+4.67%) ⬆️
src/modules/phone/index.ts 100.00% <ø> (ø)
src/faker.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/modules/address/index.ts 99.78% <100.00%> (-0.05%) ⬇️
src/modules/company/index.ts 100.00% <100.00%> (ø)
... and 6 more

@xDivisionByZerox xDivisionByZerox added the p: 1-normal Nothing urgent label Jun 11, 2022
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

The description text should include a hint that the user might want to use toFixed on the result and some reference how much another digit is in precision. E.g. 1 degree = 100 km. 1km = 0.01degree/ToFixed(2). We dont have to use precises values but some rough estimates. I guessed these values so you should probably verify them first.

src/modules/address/index.ts Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member Author

The description text should include a hint that the user might want to use toFixed on the result and some reference how much another digit is in precision. E.g. 1 degree = 100 km. 1km = 0.01degree/ToFixed(2). We dont have to use precises values but some rough estimates. I guessed these values so you should probably verify them first.

Don't know about this. That wasn't the case before either. IMO, this is on the user side, since our responsibility is the (fake) data generation not being a wiki on how specific data should be handled. If someone requests GPS coordinates I would expect them to know how to work with them.

ST-DDT
ST-DDT previously approved these changes Jun 13, 2022
@ST-DDT ST-DDT requested a review from a team June 13, 2022 19:51
@ST-DDT
Copy link
Member

ST-DDT commented Jun 13, 2022

Considering the other PR, should we rather introduce a format parameter?

@xDivisionByZerox
Copy link
Member Author

Considering the other PR, should we rather introduce a format parameter?

It doesn't make sense to me that coordinates should be strings. We simply allow the input of stringified numbers due to the nature of JS. At least that's what I like to think.

@ST-DDT
Copy link
Member

ST-DDT commented Jun 13, 2022

It doesn't make sense to me that coordinates should be strings. We simply allow the input of stringified numbers due to the nature of JS. At least that's what I like to think.

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

@xDivisionByZerox xDivisionByZerox force-pushed the refactor/address/nearbyGPSCoordinate-return-type branch from 4cfbffb to ce4cd95 Compare July 1, 2022 13:05
@xDivisionByZerox xDivisionByZerox added m: location Something is referring to the location module and removed s: accepted Accepted feature / Confirmed bug labels 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
@xDivisionByZerox xDivisionByZerox force-pushed the refactor/address/nearbyGPSCoordinate-return-type branch from df391e3 to ce9e7a5 Compare October 13, 2022 16:06
@xDivisionByZerox xDivisionByZerox removed the needs rebase There is a merge conflict label Oct 13, 2022
@xDivisionByZerox xDivisionByZerox force-pushed the refactor/address/nearbyGPSCoordinate-return-type branch from ce9e7a5 to 0ec6d01 Compare October 13, 2022 16:27
@Shinigami92 Shinigami92 changed the title refactor(address.nearbyGPSCoordinate)!: return type to number tuple refactor(location)!: change nearbyGPSCoordinate to return number tuple Oct 13, 2022
@Shinigami92
Copy link
Member

Shinigami92 commented Oct 13, 2022

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 changed the title refactor(location)!: change nearbyGPSCoordinate to return number tuple feat(location)!: change nearbyGPSCoordinate to return number tuple Oct 14, 2022
@Shinigami92 Shinigami92 changed the title feat(location)!: change nearbyGPSCoordinate to return number tuple feat(location)!: nearbyGPSCoordinate returns number tuple Oct 14, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) October 14, 2022 21:56
@Shinigami92 Shinigami92 merged commit 4765336 into next Oct 14, 2022
@ST-DDT ST-DDT deleted the refactor/address/nearbyGPSCoordinate-return-type branch November 8, 2022 23:14
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