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(finance): rename mask to maskedNumber #2055

Merged
merged 3 commits into from
Apr 20, 2023
Merged

refactor(finance): rename mask to maskedNumber #2055

merged 3 commits into from
Apr 20, 2023

Conversation

Shinigami92
Copy link
Member

references #1346 (comment)

@Shinigami92 Shinigami92 added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: finance Something is referring to the finance module labels Apr 15, 2023
@Shinigami92 Shinigami92 self-assigned this Apr 15, 2023
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 15, 2023 17:36
@xDivisionByZerox xDivisionByZerox mentioned this pull request Apr 15, 2023
Closed
@codecov
Copy link

codecov bot commented Apr 15, 2023 •
edited
Loading

Codecov Report

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

❗ Current head e748555 differs from pull request most recent head 9d1b6ed. Consider uploading reports for the commit 9d1b6ed to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             next    #2055   +/-   ##
=======================================
  Coverage        ?   99.61%           
=======================================
  Files           ?     2535           
  Lines           ?   242176           
  Branches        ?     1290           
=======================================
  Hits            ?   241240           
  Misses          ?      909           
  Partials        ?       27

@Shinigami92 Shinigami92 force-pushed the rename-mask-to-mask-number branch from 535e07a to cce244e Compare April 15, 2023 17:55
@Shinigami92 Shinigami92 changed the title refactor(finance): rename mask to maskNumber refactor(finance): rename mask to maskedNumber Apr 15, 2023
@Shinigami92 Shinigami92 requested a review from ST-DDT April 15, 2023 17:55
@ST-DDT ST-DDT requested review from a team April 15, 2023 21:29
@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Apr 15, 2023
ST-DDT
ST-DDT requested changes Apr 15, 2023
View reviewed changes
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.

Please include this in the upgrading guide.

@matthewmayer
Copy link
Contributor

Please include this in the upgrading guide.

i dont think the upgrading guide has to mention every deprecation, only

  • breaking changes
  • major module renames like name to person
  • unexpected behavior changes

if a method is just changed to a better name within the same module, and the deprecation workflow is used, i think its fine to just mention this as a breaking change in the v9 upgrading guide when it is actually removed.

@Shinigami92 Shinigami92 requested a review from a team April 16, 2023 10:35
@ST-DDT
Copy link
Member

ST-DDT commented Apr 16, 2023 •
edited
Loading

i dont think the upgrading guide has to mention every deprecation

Not individually, but maybe as a list of renamed methods.

ST-DDT
ST-DDT previously approved these changes Apr 16, 2023
View reviewed changes
@Shinigami92
Copy link
Member Author

@matthewmayer @ST-DDT I cannot find it right now, but I thought we have an open TODO/issue to re-evaluate the upgrading guideline in total before releasing v8.0
Then we should check everything over and unify some stuff and so on (in the upgrading.md)

@matthewmayer
Copy link
Contributor

It would be good to have some guidelines on what needs to be in upgrading and what doesn't per my other comment.

@Shinigami92
Copy link
Member Author

It would be good to have some guidelines on what needs to be in upgrading and what doesn't per my other comment.

I suggest moving this into its own discussion, discord chat or even issue

@matthewmayer
Copy link
Contributor

Let's make an issue

@Shinigami92 Shinigami92 mentioned this pull request Apr 16, 2023
Merged
@ST-DDT ST-DDT requested review from a team April 16, 2023 12:43
@Shinigami92 Shinigami92 dismissed stale reviews from xDivisionByZerox and ST-DDT via 9d1b6ed April 18, 2023 15:29
@Shinigami92 Shinigami92 force-pushed the rename-mask-to-mask-number branch from e748555 to 9d1b6ed Compare April 18, 2023 15:29
@ST-DDT ST-DDT requested review from a team April 18, 2023 20:58
@Shinigami92 Shinigami92 merged commit 2098b4d into next Apr 20, 2023
@Shinigami92 Shinigami92 deleted the rename-mask-to-mask-number branch April 20, 2023 15:09
@ST-DDT ST-DDT mentioned this pull request Oct 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

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

Assignees

@Shinigami92 Shinigami92

Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: finance Something is referring to the finance module p: 1-normal Nothing urgent
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.
4 participants
@Shinigami92 @matthewmayer @ST-DDT @xDivisionByZerox