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: datatype.hexadecimal signature change #1238

Merged
merged 20 commits into from
Aug 10, 2022
Merged

Conversation

import-brain
Copy link
Member

closes #954

@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent m: datatype Something is referring to the datatype module labels Aug 7, 2022
@import-brain import-brain added this to the v7 - Current Major milestone Aug 7, 2022
@import-brain import-brain self-assigned this Aug 7, 2022
@import-brain import-brain added the breaking change Cannot be merged when next version is not a major release label Aug 7, 2022
@import-brain import-brain force-pushed the feat/hex-signature-change branch from 7befc35 to c5a100b Compare August 7, 2022 22:42
@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #1238 (3ad814f) into main (d4cfe97) will decrease coverage by 0.01%.
The diff coverage is 76.08%.

@@            Coverage Diff             @@
##             main    #1238      +/-   ##
==========================================
- Coverage   99.63%   99.62%   -0.02%     
==========================================
  Files        2156     2156              
  Lines      236988   237025      +37     
  Branches     1007     1007              
==========================================
+ Hits       236124   236133       +9     
- Misses        843      871      +28     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/datatype/index.ts 96.03% <71.79%> (-3.03%) ⬇️
src/modules/color/index.ts 99.71% <100.00%> (ø)
src/modules/database/index.ts 100.00% <100.00%> (ø)
src/modules/finance/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 83.59% <0.00%> (-4.50%) ⬇️

@import-brain import-brain marked this pull request as ready for review August 7, 2022 23:41
@import-brain import-brain requested a review from a team as a code owner August 7, 2022 23:41
@import-brain import-brain requested a review from a team August 7, 2022 23:41
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.

This is a breaking change without our usual deprecation mechanics.

src/modules/finance/index.ts Outdated Show resolved Hide resolved
Minozzzi
Minozzzi previously approved these changes Aug 8, 2022
This should no longer be a breaking change.
@import-brain import-brain removed the breaking change Cannot be merged when next version is not a major release label Aug 8, 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.

One minor change then it looks good to go.

src/modules/datatype/index.ts Outdated Show resolved Hide resolved
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
ST-DDT
ST-DDT previously approved these changes Aug 8, 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.

Docs Preview (Click to expand)

grafik

@ST-DDT ST-DDT requested a review from a team August 8, 2022 22:41
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Shinigami <chrissi92@hotmail.de>
ST-DDT
ST-DDT previously approved these changes Aug 9, 2022
@ST-DDT ST-DDT requested a review from a team August 9, 2022 15:36
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Aug 9, 2022
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@Shinigami92 Shinigami92 enabled auto-merge (squash) August 10, 2022 07:26
@Shinigami92 Shinigami92 merged commit 8cb6027 into main Aug 10, 2022
@ST-DDT ST-DDT deleted the feat/hex-signature-change branch August 12, 2022 10:20
@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Aug 26, 2022

This is a breaking change without our usual deprecation mechanics.

Why did this got merged then?
@import-brain you removed the BREAKING CHANGE label. Why that?

This PR has an incorrect type as well.

@Shinigami92
Copy link
Member

This is a breaking change without our usual deprecation mechanics.

Why did this got merged then? @import-brain you removed the BREAKING CHANGE label. Why that?

This PR has an incorrect type as well.

I assume it was just one or few days before we finally defined how we want changelogs to be formatted
so this was one of the last that slept into the changelogs

@xDivisionByZerox
Copy link
Member

This explains the PR type (which is my minor concern).

Currently, this BREAKING CHANGE is in the main branch but we plan to release a new minor version on Monday. That's what I'm more concerned about.

@import-brain
Copy link
Member Author

import-brain commented Aug 26, 2022

This is a breaking change without our usual deprecation mechanics.

Why did this got merged then? @import-brain you removed the BREAKING CHANGE label. Why that?

This PR has an incorrect type as well.

I changed the code such that it wasn't a breaking change anymore.

https://github.com/faker-js/faker/pull/1238/files#diff-7024ee9abbac290279ea20612a309bc1fa8ba9d0db8f31f6ba87482e9f930761L196-R222

The function signature used to take just one param, length (a number), and I changed it so that it takes a param called options that could be an options object OR a number and called the deprecation function when options was a number.

@xDivisionByZerox
Copy link
Member

Yeah but you changed the default return value to not include the prefix.
Currently, I can do this:

const hex = faker.datatype.hexadecimal(1);
console.log(parseInt(hex)); // 14

This will break now:

const hex = faker.datatype.hexadecimal(1);
console.log(parseInt(hex)); // NaN

@import-brain
Copy link
Member Author

Yeah but you changed the default return value to not include the prefix. Currently, I can do this:

const hex = faker.datatype.hexadecimal(1);
console.log(parseInt(hex)); // 14

This will break now:

const hex = faker.datatype.hexadecimal(1);
console.log(parseInt(hex)); // NaN

Oh crap. Should I manually revert this and reimplement the change before we release v8?

@ST-DDT
Copy link
Member

ST-DDT commented Aug 27, 2022

I wouldnt revert it entirely, just restore the old defaults.

@xDivisionByZerox Nicely spotted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: datatype Something is referring to the datatype module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Change signature of hex
5 participants