Skip to content

refactor(location): normalize signatures #1784

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
Merged
merged 18 commits into from
Feb 4, 2023
Merged

refactor(location): normalize signatures #1784

merged 18 commits into from
Feb 4, 2023

Conversation

xDivisionByZerox
Copy link
Member

@xDivisionByZerox xDivisionByZerox commented Jan 26, 2023 •
edited
Loading

Part of #1349.

Standardize all function signatures in the LocationModule to support an options object.

@xDivisionByZerox xDivisionByZerox added s: on hold Blocked by something or frozen to avoid conflicts 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 labels Jan 26, 2023
@xDivisionByZerox xDivisionByZerox self-assigned this Jan 26, 2023
@xDivisionByZerox xDivisionByZerox linked an issue Jan 26, 2023 that may be closed by this pull request
Standardize function parameters and defaults #1349
Closed
@codecov
Copy link

codecov bot commented Jan 27, 2023 •
edited
Loading

Codecov Report

Merging #1784 (4e25d9f) into next (62fc5ef) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #1784      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2346     2346              
  Lines      235103   235497     +394     
  Branches     1129     1135       +6     
==========================================
+ Hits       234228   234619     +391     
- Misses        853      856       +3     
  Partials       22       22
Impacted Files Coverage Δ
src/modules/location/index.ts 98.70% <100.00%> (+0.77%) ⬆️
src/modules/internet/user-agent.ts 87.57% <0.00%> (-0.60%) ⬇️

ST-DDT
ST-DDT reviewed Jan 27, 2023
View reviewed changes
@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Jan 28, 2023 •
edited
Loading

The PR is now open for review. For simplicity (on my side) I literally took the previous parameter and simply wrapped it in an options object.

I highly encourage all reviewers to discuss the "new" property names in the options argument for each function. If you like a suggestion from an other reviewer, please upvote the comment. I'll leave each comment hanging around for some time before actually changing the discussed object property names.

@xDivisionByZerox xDivisionByZerox marked this pull request as ready for review January 28, 2023 14:01
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner January 28, 2023 14:01
@xDivisionByZerox xDivisionByZerox requested a review from a team January 28, 2023 14:03
@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent and removed s: on hold Blocked by something or frozen to avoid conflicts labels Jan 28, 2023
Shinigami92
Shinigami92 reviewed Jan 28, 2023
View reviewed changes
ST-DDT
ST-DDT requested changes Feb 1, 2023
View reviewed changes
@xDivisionByZerox xDivisionByZerox requested review from a team, ST-DDT and Shinigami92 February 2, 2023 16:38
@ST-DDT ST-DDT requested a review from a team February 2, 2023 21:19
Shinigami92
Shinigami92 approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

I assume the deprecation will be done in another PR for better reviewability?

@xDivisionByZerox
Copy link
Member Author

I assume the deprecation will be done in another PR for better reviewability?

Actually, I wanted to deprecate each method in its own PR so it is visible in the changelog.

@Shinigami92 Shinigami92 requested a review from ejcheng February 3, 2023 09:10
@Shinigami92 Shinigami92 enabled auto-merge (squash) February 4, 2023 00:09
@Shinigami92 Shinigami92 merged commit 8e4526d into next Feb 4, 2023
@Shinigami92 Shinigami92 deleted the refactor/location/normalize-signatures branch February 4, 2023 00:18
matthewmayer pushed a commit to matthewmayer/faker that referenced this pull request Feb 18, 2023
@xDivisionByZerox @matthewmayer
refactor(location): normalize signatures (faker-js#1784)
b193fc9
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

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@ejcheng ejcheng Awaiting requested review from ejcheng

Assignees

@xDivisionByZerox xDivisionByZerox

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.

Standardize function parameters and defaults
3 participants
@xDivisionByZerox @ST-DDT @Shinigami92