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

feat(commerce): Add method for generating ISBN-10 and ISBN-13 #2240

Merged
merged 20 commits into from
Sep 10, 2023
Merged

feat(commerce): Add method for generating ISBN-10 and ISBN-13 #2240

merged 20 commits into from
Sep 10, 2023

Conversation

RobinvanderVliet
Copy link
Contributor

This is my first pull request contributing some actual code to this project, so any feedback is welcome!

This merge request resolves issue #2196.

@RobinvanderVliet RobinvanderVliet requested a review from a team as a code owner July 5, 2023 22:43
@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature m: commerce Something is referring to the commerce module labels Jul 6, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team July 6, 2023 20:18
@ST-DDT ST-DDT added the s: waiting for user interest Waiting for more users interested in this feature label Jul 9, 2023
ST-DDT
ST-DDT reviewed Jul 9, 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 note that the associated issue is marked as waiting for user interest, so this PR might not be merged immediately.

src/modules/commerce/index.ts Outdated Show resolved Hide resolved
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
test/commerce.spec.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT linked an issue Jul 9, 2023 that may be closed by this pull request
ISBN 10 and ISBN13 #2196
Closed
@codecov
Copy link

codecov bot commented Jul 9, 2023 •
edited
Loading

Codecov Report

Merging #2240 (3201b18) into next (8a6ce49) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@           Coverage Diff            @@
##             next    #2240    +/-   ##
========================================
  Coverage   99.60%   99.61%            
========================================
  Files        2786     2786            
  Lines      252522   252674   +152     
  Branches     1083     1096    +13     
========================================
+ Hits       251529   251699   +170     
+ Misses        966      948    -18     
  Partials       27       27
Files Changed Coverage Δ
src/modules/commerce/index.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

ST-DDT
ST-DDT reviewed Jul 19, 2023
View reviewed changes
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Jul 20, 2023
View reviewed changes
src/modules/commerce/index.ts Outdated Show resolved Hide resolved
@RobinvanderVliet @ST-DDT
Update @since
2a6552f 
Co-authored-by: ST-DDT <[email protected]>
ST-DDT
ST-DDT previously approved these changes Jul 20, 2023
View reviewed changes
@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug and removed s: waiting for user interest Waiting for more users interested in this feature labels Aug 10, 2023
@xDivisionByZerox
Copy link
Member

Team decision

We accept this feature request.

@RobinvanderVliet Please resolve to merge conflicts so we can open it for review.

@xDivisionByZerox xDivisionByZerox added the needs rebase There is a merge conflict label Aug 10, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Aug 14, 2023

@RobinvanderVliet Could you please fix the merge conflicts?

@RobinvanderVliet
Copy link
Contributor Author

I resolved the merge conflicts!

@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Aug 14, 2023
@ST-DDT ST-DDT requested a review from a team August 28, 2023 22:06
xDivisionByZerox
xDivisionByZerox approved these changes Sep 9, 2023
View reviewed changes
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

I’m considering the use of a strategy pattern to improve the maintainability of the changed code. However, I have decided to accept the current implementation.

@ST-DDT ST-DDT merged commit cb4ef28 into faker-js:next Sep 10, 2023
36 checks passed
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

@import-brain import-brain import-brain approved these changes

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

Assignees

@RobinvanderVliet RobinvanderVliet

Labels
c: feature Request for new feature m: commerce Something is referring to the commerce module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Milestone
v8.x
Development

Successfully merging this pull request may close these issues.

ISBN 10 and ISBN13
4 participants
@RobinvanderVliet @xDivisionByZerox @ST-DDT @import-brain