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

Merged
merged 21 commits into from
Oct 10, 2024
Merged

Conversation

cieslarmichal
Copy link
Contributor

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!

Name Link
🔨 Latest commit 76987e0
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/6707f48d5378110008b87b9d
😎 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.

@cieslarmichal cieslarmichal mentioned this pull request Jun 15, 2024
@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 (1760658) to head (76987e0).
Report is 1 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2949      +/-   ##
==========================================
- Coverage   99.96%   99.96%   -0.01%     
==========================================
  Files        2778     2794      +16     
  Lines      226364   227415    +1051     
  Branches      577      575       -2     
==========================================
+ Hits       226288   227333    +1045     
- Misses         76       82       +6     
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
import-brain
import-brain previously approved these changes Jun 20, 2024
@cieslarmichal
Copy link
Contributor 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
Contributor 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 😎

@ST-DDT
Copy link
Member

ST-DDT commented Sep 27, 2024

Dear @cieslarmichal,

Thank you for your patience and contributions so far. We wanted to provide you with a brief update.

We are pleased to inform you that we have addressed all the known bugs in version 9.0. We will now take a short break and resume work around October 7, 2024, to begin merging new features for v9.1, including yours.

In the meantime, we hope you have a great time, and we sincerely appreciate your ongoing support and contributions.

Best regards,
your FakerJS Team

@cieslarmichal
Copy link
Contributor Author

Hello, thank you for giving me a chance to contribute and for the information about expected release date 💪

@Shinigami92 Shinigami92 merged commit 2f93d9d into faker-js:next Oct 10, 2024
23 checks passed
@cieslarmichal cieslarmichal deleted the feature/book-module branch October 10, 2024 16:02
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