Skip to content

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

Merged
ST-DDT merged 20 commits intofaker-js:nextfrom
RobinvanderVliet:add-isbn
Sep 10, 2023
Merged

feat(commerce): Add method for generating ISBN-10 and ISBN-13#2240
ST-DDT merged 20 commits intofaker-js:nextfrom
RobinvanderVliet:add-isbn

Conversation

@RobinvanderVliet
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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.

Comment thread src/modules/commerce/index.ts Outdated
Comment thread src/modules/commerce/index.ts Outdated
Comment thread test/commerce.spec.ts
@ST-DDT ST-DDT linked an issue Jul 9, 2023 that may be closed by this pull request
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 9, 2023

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

Comment thread src/modules/commerce/index.ts Outdated
Comment thread src/modules/commerce/index.ts Outdated
Comment thread src/modules/commerce/index.ts
Comment thread src/modules/commerce/index.ts Outdated
Comment thread src/modules/commerce/index.ts Outdated
Comment thread src/modules/commerce/index.ts Outdated
Comment thread src/modules/commerce/index.ts Outdated
ST-DDT
ST-DDT previously approved these changes Jul 20, 2023
Comment thread src/modules/commerce/index.ts Outdated
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
ST-DDT
ST-DDT previously approved these changes Jul 20, 2023
@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
Copy Markdown
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
Copy Markdown
Member

ST-DDT commented Aug 14, 2023

@RobinvanderVliet Could you please fix the merge conflicts?

@RobinvanderVliet
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
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: commerce Something is referring to the commerce module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ISBN 10 and ISBN13

4 participants