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(string)!: swap allowLeadingZeros default to true #1602

Merged
merged 17 commits into from
Dec 6, 2022
Merged

refactor(string)!: swap allowLeadingZeros default to true #1602

merged 17 commits into from
Dec 6, 2022

Conversation

ejcheng
Copy link
Member

@ejcheng ejcheng commented Nov 25, 2022

closes #1592

@ejcheng ejcheng added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug 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 m: string Something is referring to the string module labels Nov 25, 2022
@ejcheng ejcheng added this to the v8.0 - Module Re-Shuffling milestone Nov 25, 2022
@ejcheng ejcheng requested a review from a team November 25, 2022 05:39
@ejcheng ejcheng self-assigned this Nov 25, 2022
@ejcheng ejcheng requested a review from a team as a code owner November 25, 2022 05:39
@codecov
Copy link

codecov bot commented Nov 25, 2022 •
edited
Loading

Codecov Report

Merging #1602 (cf8b1db) into next (6bac476) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             next    #1602   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files        2222     2222           
  Lines      240430   240434    +4     
  Branches     1064     1064           
=======================================
+ Hits       239528   239532    +4     
  Misses        881      881           
  Partials       21       21
Impacted Files Coverage Δ
src/modules/helpers/index.ts 98.55% <100.00%> (ø)
src/modules/number/index.ts 100.00% <100.00%> (ø)
src/modules/random/index.ts 99.00% <100.00%> (ø)
src/modules/string/index.ts 100.00% <100.00%> (ø)

Shinigami92
Shinigami92 requested changes Nov 25, 2022
View reviewed changes
@ejcheng ejcheng requested a review from Shinigami92 November 25, 2022 16:52
@matthewmayer
Copy link
Contributor

Could you include a change to docs/guide/upgrading.md as part of this PR for documenting this behavior change for anyone upgrading from v7 to v8?

ST-DDT
ST-DDT requested changes Nov 26, 2022
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.

Should we add a link from string.numeric -> number.int (and vice versa)?

@ST-DDT ST-DDT added the needs documentation Documentations are needed label Nov 26, 2022
@ejcheng
Copy link
Member Author

ejcheng commented Nov 26, 2022

Should we add a link from string.numeric -> number.int (and vice versa)?

What do you mean by link? A jsdoc @link tag?

@ejcheng ejcheng requested a review from a team November 26, 2022 19:14
@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2022 •
edited
Loading

Should we add a link from string.numeric -> number.int (and vice versa)?

What do you mean by link? A jsdoc @link tag?

I'm referring to @see, so the user is aware that they can get a number (within a range) or just a bunch of digits depending on their requirements.

ST-DDT
ST-DDT reviewed Nov 26, 2022
View reviewed changes
ST-DDT
ST-DDT reviewed Nov 26, 2022
View reviewed changes
@ejcheng ejcheng requested a review from ST-DDT November 27, 2022 21:12
ST-DDT
ST-DDT previously approved these changes Nov 27, 2022
View reviewed changes
matthewmayer
matthewmayer requested changes Nov 28, 2022
View reviewed changes
@ejcheng ejcheng requested review from matthewmayer and ST-DDT and removed request for matthewmayer December 3, 2022 19:46
ST-DDT
ST-DDT reviewed Dec 3, 2022
View reviewed changes
@ejcheng ejcheng requested review from ST-DDT and matthewmayer and removed request for matthewmayer December 5, 2022 12:38
@ST-DDT ST-DDT removed the needs documentation Documentations are needed label Dec 5, 2022
@ST-DDT ST-DDT merged commit c4b7ce8 into next Dec 6, 2022
@ST-DDT ST-DDT deleted the refactor/swap_allowLeadingZeros_default branch December 6, 2022 23:13
@ST-DDT
Copy link
Member

ST-DDT commented Dec 7, 2022

@import-brain Looking at the now failing PRs, it looks like we missed some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewmayer matthewmayer matthewmayer approved these changes

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

@Shinigami92 Shinigami92 Shinigami92 approved these changes

Assignees

@ejcheng ejcheng

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: string Something is referring to the string 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.

Swap allowLeadingZeros default value to true
4 participants
@ejcheng @matthewmayer @ST-DDT @Shinigami92