Skip to content

feat(locale): Add bn_BD locale #3439

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

Merged
merged 10 commits into from
Mar 16, 2025
Merged

Conversation

AbrarShahriar
Copy link
Contributor

@AbrarShahriar AbrarShahriar commented Mar 13, 2025

Added Bengali locale.

Modified Modules:

  • Date

@AbrarShahriar AbrarShahriar requested a review from a team as a code owner March 13, 2025 22:23
Copy link

netlify bot commented Mar 13, 2025

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 55ae211
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/67d533dc0dbe3e000864e527
😎 Deploy Preview https://deploy-preview-3439.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.

@AbrarShahriar AbrarShahriar changed the title feat(local): Add bn locale feat(locale): Add bn locale Mar 13, 2025
Copy link

codecov bot commented Mar 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (50b3241) to head (55ae211).
Report is 1 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3439      +/-   ##
==========================================
- Coverage   99.97%   99.97%   -0.01%     
==========================================
  Files        2811     2817       +6     
  Lines      217414   217493      +79     
  Branches      951      951              
==========================================
+ Hits       217361   217438      +77     
- Misses         53       55       +2     
Files with missing lines Coverage Δ
src/locale/bn_BD.ts 100.00% <100.00%> (ø)
src/locale/index.ts 100.00% <100.00%> (ø)
src/locales/bn_BD/date/index.ts 100.00% <100.00%> (ø)
src/locales/bn_BD/date/month.ts 100.00% <100.00%> (ø)
src/locales/bn_BD/date/weekday.ts 100.00% <100.00%> (ø)
src/locales/bn_BD/index.ts 100.00% <100.00%> (ø)
src/locales/bn_BD/metadata.ts 100.00% <100.00%> (ø)
src/locales/index.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@matthewmayer
Copy link
Contributor

In order to make it easier to review new locales, we generally ask for pull requests to contain a smaller number of files, and focus on a single module at a time.

Could I suggest you make a new pull request which just contains

  • the Bengali metadata
  • one simple locale data file (e.g. "date" or "animal")

Once that is approved you can follow up with additional PRs to add the remaining data.

Also we should decide if we want the locale code to be bn or bn_BD (Bangladesh) - since the phone numbers etc (and the cities?) are country-specific, it should probably be bn_BD.

@AbrarShahriar
Copy link
Contributor Author

@matthewmayer Sure! I’ve updated the PR based on your feedback:

  • Removed all modules except date.
  • Renamed the locale code to bn_BD since phone formats and locations are Bangladesh-specific. Let me know if something else has been decided.
  • Updated the metadata to reflect Bengali (Bangladesh).

@AbrarShahriar AbrarShahriar changed the title feat(locale): Add bn locale feat(locale): Add bn_BD locale Mar 14, 2025
@AbrarShahriar
Copy link
Contributor Author

@matthewmayer Hey, made the changes you said. Although after running preflight, a few entries were added with the key bn_BD and I tried to manually fix them.

This PR might've become a little bit too messy for review. If you encounter any issues or suggest any changes, let me know. Also if this PR is becoming troublesome, I can open another PR with the changes you mentioned.

@matthewmayer
Copy link
Contributor

It's fine. Let's keep it in one PR. We always squash-merge when approving so all the little tweaks don't appear in the main history.

I recommend looking at the files view on GitHub to check all changes for your PR. Then it's easier to see if there are any accidental changes that need reverting.

Thanks for your contributions!

@AbrarShahriar
Copy link
Contributor Author

@matthewmayer I reverted the changes as you mentioned. Hopefully it should be okay now.

@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature p: 1-normal Nothing urgent c: locale Permutes locale definitions m: date Something is referring to the date module labels Mar 15, 2025
@xDivisionByZerox xDivisionByZerox added this to the vAnytime milestone Mar 15, 2025
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.

Thank you for your contribution and the great cooperation with matt ❤️
I'll leave the PR open for at least another 24 hours, so other team members have the chance of reviewing as well.

@xDivisionByZerox xDivisionByZerox requested review from a team March 15, 2025 20:39
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

LGTM (does not include a bitcoin miner 😜)

@AbrarShahriar
Copy link
Contributor Author

@xDivisionByZerox Thank you, glad to be of some help<3

@AbrarShahriar
Copy link
Contributor Author

@Shinigami92 Haha, relieved it passed the crypto-miner test! 😆

@xDivisionByZerox xDivisionByZerox merged commit fef0ad7 into faker-js:next Mar 16, 2025
27 checks passed
@matthewmayer
Copy link
Contributor

@AbrarShahriar you can follow up with additional PRs for more modules now

For ease of review please make sure

  • you branch from the current HEAD of the next branch each time
  • you only propose data for one module, or a small group of related modules, at a time
  • you list the source of the data e.g. if its your own research, or uses AI, or from any official published lists etc - so we can check for any copyright issues
  • you sort all string arrays alphabetically (with a naive Unicode sort) - only certain modules require this but its good practice for all new modules

@AbrarShahriar
Copy link
Contributor Author

@matthewmayer Thanks for the guidance!
I had a query. Do I create a new branch from the current HEAD of the next branch for each module update, or do I work on the current HEAD of the next branch directly?

@matthewmayer
Copy link
Contributor

Suggest a new branch for each change based on the current HEAD of next.

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 c: locale Permutes locale definitions m: date Something is referring to the date module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants