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(person): add Portuguese BR Language to western zodiac sign #1814

Merged
merged 3 commits into from
Feb 11, 2023

Conversation

Minozzzi
Copy link
Contributor

@Minozzzi Minozzzi commented Feb 6, 2023

No description provided.

@Minozzzi Minozzzi requested a review from a team as a code owner February 6, 2023 11:31
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.

you need to run pnpm run generate:locales to update the src/locales/pt_BR/person/index.ts file

@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Merging #1814 (5721f26) into next (99308d4) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 5721f26 differs from pull request most recent head 7c8c70d. Consider uploading reports for the commit 7c8c70d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1814      +/-   ##
==========================================
- Coverage   99.64%   99.63%   -0.01%     
==========================================
  Files        2346     2347       +1     
  Lines      235646   235662      +16     
  Branches     1142     1142              
==========================================
+ Hits       234798   234806       +8     
- Misses        826      834       +8     
  Partials       22       22              
Impacted Files Coverage Δ
src/locales/pt_BR/person/index.ts 100.00% <100.00%> (ø)
src/locales/pt_BR/person/western_zodiac_sign.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 94.08% <0.00%> (-1.78%) ⬇️
src/modules/location/index.ts 98.70% <0.00%> (-0.22%) ⬇️

Copy link
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.

Looks good to me.
We should consider sorting these alphabetically as well (for overall easier review-ability of all locale data), although it might be detrimental for this data set in particular.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug c: locale Permutes locale definitions m: person Something is referring to the person module labels Feb 6, 2023
@matthewmayer
Copy link
Contributor

Looks good to me.

We should consider sorting these alphabetically as well (for overall easier review-ability of all locale data), although it might be detrimental for this data set in particular.

As zodiac signs have an natural order (like months) it seems more sensible to keep them in that order.

@ST-DDT ST-DDT requested review from a team February 8, 2023 00:12
@ST-DDT ST-DDT enabled auto-merge (squash) February 11, 2023 11:07
@ST-DDT ST-DDT merged commit fd96094 into faker-js:next Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions m: person Something is referring to the person module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants