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: add book module #2949

Open
wants to merge 16 commits into
base: next
Choose a base branch
from

Conversation

cieslarmichal
Copy link

Added new Book module as discussed in #2310.

Changes made:

  • added a new module with following methods: author, format, genre, publisher, series, title
  • added en locale data
  • added pl locale data
  • added tests for all methods
  • added new docs page

@cieslarmichal cieslarmichal requested a review from a team as a code owner June 15, 2024 16:53
Copy link

netlify bot commented Jun 15, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4561d0a
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66dec0b88291000008f1d7ee
😎 Deploy Preview https://deploy-preview-2949.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ST-DDT ST-DDT linked an issue Jun 15, 2024 that may be closed by this pull request
@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent labels Jun 15, 2024
@ST-DDT ST-DDT added this to the v9.x milestone Jun 15, 2024
CONTRIBUTING.md Outdated Show resolved Hide resolved
src/definitions/book.ts Outdated Show resolved Hide resolved
src/locales/en/book/publisher.ts Outdated Show resolved Hide resolved
src/modules/book/index.ts Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (ca9d036) to head (4561d0a).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2949      +/-   ##
==========================================
- Coverage   99.96%   99.96%   -0.01%     
==========================================
  Files        2776     2792      +16     
  Lines      226260   227311    +1051     
  Branches      939      585     -354     
==========================================
+ Hits       226180   227230    +1050     
- Misses         80       81       +1     
Files with missing lines Coverage Δ
src/definitions/book.ts 100.00% <100.00%> (ø)
src/definitions/definitions.ts 100.00% <ø> (ø)
src/definitions/index.ts 100.00% <ø> (ø)
src/faker.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <ø> (ø)
src/locales/en/book/author.ts 100.00% <100.00%> (ø)
src/locales/en/book/format.ts 100.00% <100.00%> (ø)
src/locales/en/book/genre.ts 100.00% <100.00%> (ø)
src/locales/en/book/index.ts 100.00% <100.00%> (ø)
src/locales/en/book/publisher.ts 100.00% <100.00%> (ø)
... and 12 more

... and 1 file with indirect coverage changes

@ST-DDT ST-DDT requested review from a team June 15, 2024 17:35
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug m: book Something is referring to the animal module labels Jun 15, 2024
ST-DDT
ST-DDT previously approved these changes Jun 15, 2024
@ST-DDT ST-DDT requested a review from a team June 15, 2024 19:24
import-brain
import-brain previously approved these changes Jun 16, 2024
src/locales/en/book/author.ts Outdated Show resolved Hide resolved
src/locales/en/book/genre.ts Outdated Show resolved Hide resolved
src/locales/en/book/genre.ts Outdated Show resolved Hide resolved
src/locales/en/book/genre.ts Outdated Show resolved Hide resolved
src/locales/en/book/genre.ts Outdated Show resolved Hide resolved
src/locales/en/book/title.ts Outdated Show resolved Hide resolved
src/locales/en/book/title.ts Show resolved Hide resolved
src/locales/pl/book/author.ts Show resolved Hide resolved
src/modules/book/index.ts Show resolved Hide resolved
src/modules/book/index.ts Outdated Show resolved Hide resolved
@cieslarmichal cieslarmichal dismissed stale reviews from import-brain and ST-DDT via ffee57d June 16, 2024 10:14
@xDivisionByZerox
Copy link
Member

hello guys, is there anything more I should do here? 🙂

There might be. If there is we will write a comment. 🙂
It's not unusual in open source that PRs will take a while to be fully reviewed and merged.

Please keep in mind that all members and maintainers of FakerJS work on this project in their free time.

@cieslarmichal
Copy link
Author

I know, sure, just asking :)

matthewmayer
matthewmayer previously approved these changes Jun 18, 2024
Copy link
Contributor

@matthewmayer matthewmayer left a comment

Choose a reason for hiding this comment

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

Happy with the API I think it looks good overall!

Maybe we could have a few more books for the title method, I feel 100 is not that many. We normally cap any one data type at 1000. Perhaps we could get up to 200 books?

Non urgent as this probably won't get merged until 9.1

@cieslarmichal
Copy link
Author

Ok I will add more book titles

ST-DDT
ST-DDT previously approved these changes Jun 18, 2024
Shinigami92
Shinigami92 previously approved these changes Jun 18, 2024
import-brain
import-brain previously approved these changes Jun 20, 2024
@cieslarmichal
Copy link
Author

I have just added more book titles and authors (about 150 each).

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'll already approve. I still have the question if the addition of the polish locale should be it's own PR. That way there would be a separate entry on our reloase notes/changelog.

  1. Create book module (new modules have to have applicable locale data in the default locale => en)
  2. Add polish locale data for book module.

This is not blocking. Just a question on the process.

Edit: Also, thank you @cieslarmichal for the initial waiting, quick implementation and reasonable feedback loops. Was a pleasure reviewing your PR.

@ST-DDT
Copy link
Member

ST-DDT commented Jun 22, 2024

I still have the question if the addition of the polish locale should be it's own PR.

Initial data is initial data.
IMO if I consider something useful, I would try it instead of searching the changelog for my locale.
Unless, I know the feature exists and I'm just waiting for it to get available for my locale.
But I don't have a particular strong opinion on this either.

@matthewmayer
Copy link
Contributor

matthewmayer commented Jun 23, 2024

I would say if it's only 2 or 3 locales included it's better to include in the initial commit. That helps verify there are no incorrect assumptions about how the data will be localised (for example in the first version of this PR the pl and en versions of author used different FirstName surname orders. That was good, as it helped us clarify what the rule should be in general and standardise it.)

@cieslarmichal
Copy link
Author

Thank you guys for a nice word :) It was a good review process, I am happy about it, can't wait to release those changes 😎

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

Add books module
6 participants