Skip to content

feat(location): add list of spoken languages#3333

Merged
ST-DDT merged 32 commits into
faker-js:nextfrom
UmairJibran:next
Dec 14, 2024
Merged

feat(location): add list of spoken languages#3333
ST-DDT merged 32 commits into
faker-js:nextfrom
UmairJibran:next

Conversation

@UmairJibran

@UmairJibran UmairJibran commented Dec 10, 2024

Copy link
Copy Markdown
Contributor

closes #1548
closes #2622

For Ref:

Languages added for

  • English [en]
  • Urdu [ur]
@netlify

netlify Bot commented Dec 10, 2024

Copy link
Copy Markdown

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 347f203
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/675bb9f787ad3300080dbfa3
😎 Deploy Preview https://deploy-preview-3333.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.

@codecov

codecov Bot commented Dec 10, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (5ec4a6c) to head (347f203).
Report is 3 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3333      +/-   ##
==========================================
- Coverage   99.97%   99.96%   -0.01%     
==========================================
  Files        2807     2809       +2     
  Lines      217087   217202     +115     
  Branches      969      967       -2     
==========================================
+ Hits       217029   217134     +105     
- Misses         58       68      +10     
Files with missing lines Coverage Δ
src/definitions/location.ts 100.00% <100.00%> (ø)
src/locales/en/location/index.ts 100.00% <100.00%> (ø)
src/locales/en/location/language.ts 100.00% <100.00%> (ø)
src/locales/ur/location/index.ts 100.00% <100.00%> (ø)
src/locales/ur/location/language.ts 100.00% <100.00%> (ø)
src/modules/location/index.ts 100.00% <100.00%> (+0.92%) ⬆️

... and 1 file with indirect coverage changes

@UmairJibran UmairJibran changed the title add list of spoken languages feat: add list of spoken languages Dec 10, 2024
@UmairJibran UmairJibran marked this pull request as ready for review December 10, 2024 19:49
@UmairJibran UmairJibran requested a review from a team as a code owner December 10, 2024 19:49
@UmairJibran UmairJibran changed the title feat: add list of spoken languages feat(location): add list of spoken languages Dec 10, 2024
@matthewmayer

Copy link
Copy Markdown
Contributor

I think based on discussion at #2622 we wanted this to return an object with not just the language name but also the iso 2 and 3 letter codes where available.

@UmairJibran

Copy link
Copy Markdown
Contributor Author

Thanks @matthewmayer I didn't read that discussion, I'll update it later tonight

@UmairJibran

Copy link
Copy Markdown
Contributor Author

It's ready for review

Comment thread src/modules/location/index.ts Outdated
Comment thread src/modules/location/index.ts Outdated
@ST-DDT ST-DDT added this to the vAnytime milestone Dec 11, 2024
@ST-DDT ST-DDT added the c: feature Request for new feature label Dec 11, 2024

@ST-DDT ST-DDT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your quick responses.
I have found a few more things to potentially improve this.

My biggest question mark regarding this feature is which values the method is supposed to return.

  • The languages commonly known/spoken by/relevant for the populance of a locale
  • "All" languages regardless of applicability for the locale (current)

(This is relevant for the countryCode method as well #2305)

Comment thread src/modules/location/index.ts Outdated
Comment thread src/modules/location/index.ts Outdated
Comment thread src/definitions/location.ts Outdated
Comment thread src/definitions/location.ts Outdated
Comment thread src/definitions/location.ts Outdated
Comment thread src/modules/location/index.ts Outdated
Comment thread src/modules/location/index.ts Outdated
@matthewmayer

Copy link
Copy Markdown
Contributor

I'd say all languages.

Comment thread test/modules/location.spec.ts
Comment thread src/modules/location/index.ts Outdated
@matthewmayer

Copy link
Copy Markdown
Contributor

I think we should include at least all languages faker is localised into. I think the following are missing.

  1. Afrikaans (af)
    2. Azerbaijani (az)
    3. Maldivian (dv)
    4. Farsi/Persian (fa)
    5. Latvian (lv)
    6. Indonesian (id)
    7. Nepali (ne)
    8. Thai (th)
    9. Uzbek (uz)
    10. Yoruba (yo)
@UmairJibran

Copy link
Copy Markdown
Contributor Author

I have pushed the latest changes.

@ST-DDT @matthewmayer thanks a lot guys for going through the changes in smaller iterations!

for #2305 I'm not sure how to approach it, do I hard coded which language and countryCode goes where? I could work on that but I would want to take it up in a different PR.

ST-DDT
ST-DDT previously approved these changes Dec 12, 2024
@ST-DDT ST-DDT requested review from a team December 12, 2024 21:34
@matthewmayer

Copy link
Copy Markdown
Contributor

I have pushed the latest changes.

@ST-DDT @matthewmayer thanks a lot guys for going through the changes in smaller iterations!

for #2305 I'm not sure how to approach it, do I hard coded which language and countryCode goes where? I could work on that but I would want to take it up in a different PR.

yeah dont worry about #2305 for now that would certainly be a different PR if we decide to do it

Comment thread src/modules/location/index.ts Outdated
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@ST-DDT ST-DDT added this pull request to the merge queue Dec 14, 2024
Merged via the queue into faker-js:next with commit ff6dda9 Dec 14, 2024
@ST-DDT ST-DDT modified the milestones: vAnytime, v9.x Jun 22, 2026
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: location Something is referring to the location module p: 1-normal Nothing urgent

3 participants