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

import-brain
Copy link
Member

closes #1592

@import-brain import-brain 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
@import-brain import-brain requested a review from a team November 25, 2022 05:39
@import-brain import-brain self-assigned this Nov 25, 2022
@import-brain import-brain 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

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
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
src/modules/random/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
test/string.spec.ts Outdated Show resolved Hide resolved
import-brain and others added 2 commits November 25, 2022 11:49
@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)?

src/modules/helpers/index.ts Outdated Show resolved Hide resolved
src/modules/random/index.ts Show resolved Hide resolved
@ST-DDT ST-DDT added the needs documentation Documentations are needed label Nov 26, 2022
@import-brain @ST-DDT
Update src/modules/helpers/index.ts
841979c 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@import-brain
Copy link
Member Author

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

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

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

ST-DDT commented Nov 26, 2022 •
edited

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
docs/guide/upgrading.md Outdated Show resolved Hide resolved
@import-brain @ST-DDT
Update docs/guide/upgrading.md
2aa7626 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
ST-DDT
ST-DDT reviewed Nov 26, 2022
View reviewed changes
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
Shinigami92 and others added 2 commits November 26, 2022 21:41
@Shinigami92 @ST-DDT
Update src/modules/number/index.ts
9b06a3d 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@import-brain @ST-DDT
Update src/modules/string/index.ts
6ae5e9e 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
ST-DDT
ST-DDT previously approved these changes Nov 27, 2022
View reviewed changes
matthewmayer
matthewmayer requested changes Nov 28, 2022
View reviewed changes
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
@import-brain import-brain 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
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
@import-brain import-brain 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

@import-brain import-brain

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
@import-brain @matthewmayer @ST-DDT @Shinigami92