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.

Sign up for GitHub

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

Jump to bottom

feat(locale): Add locale ro_MD #1848

Closed
wants to merge 0 commits into from
Closed

feat(locale): Add locale ro_MD #1848

wants to merge 0 commits into from

Conversation

botzill
Copy link

@botzill botzill commented Feb 16, 2023

Add a new locale for ro_MD

@botzill botzill requested a review from a team February 16, 2023 13:39
@botzill botzill requested a review from a team as a code owner February 16, 2023 13:39
@botzill botzill changed the title Add locale ro_MD feat(locale): Add locale ro_MD Feb 16, 2023
matthewmayer
matthewmayer requested changes Feb 16, 2023
View reviewed changes
src/locales/ro_MD/date/month.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/date/month.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/date/weekday.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/date/weekday.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/internet/domain_suffix.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/person/female_first_name.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/person/last_name.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/person/male_first_name.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/person/name.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/person/suffix.ts Outdated Show resolved Hide resolved
matthewmayer
matthewmayer requested changes Feb 16, 2023
View reviewed changes
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.

Also run pnpm run test and check for errors, CI reports two about duplicate entries:

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 2 ⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/locales.spec.ts > locale > ro_MD > location > city_name > should not have duplicate entries
Error: Duplicated values are [Florești]

 FAIL  test/locales.spec.ts > locale > ro_MD > person > female_first_name > should not have duplicate entries
Error: Duplicated values are [Ianna, Irina]

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: locale Permutes locale definitions labels Feb 16, 2023
matthewmayer
matthewmayer requested changes Feb 17, 2023
View reviewed changes
src/locales/ro_MD/date/weekday.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

Btw recommended you don't force push, that way it's easier to see the history of the PR. It gets squash-merged at the end anyway!

ST-DDT
ST-DDT reviewed Feb 17, 2023
View reviewed changes
src/locales/ro_MD/date/weekday.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/location/street.ts Outdated Show resolved Hide resolved
src/locales/ro_MD/location/county.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Feb 17, 2023

I'm almost done with reviewing this, I will do the rest this weekend.
Thanks for your contribution and your quick fixes ❤️ .

@botzill
Copy link
Author

botzill commented Feb 20, 2023

I'm almost done with reviewing this, I will do the rest this weekend. Thanks for your contribution and your quick fixes ❤️ .

Thanks

@botzill
Copy link
Author

botzill commented Feb 21, 2023

Hi @ST-DDT, let me know, please, if there are any other changes required.

Thanks.

Shinigami92
Shinigami92 previously approved these changes Feb 21, 2023
View reviewed changes
@Shinigami92 Shinigami92 dismissed their stale review February 21, 2023 07:14

I overlooked that there are failing tests, please fix them

@botzill
Copy link
Author

botzill commented Feb 21, 2023

How should I fix this:

❯ test/all_functional.spec.ts  (30506 tests | 6 failed) 7505ms
   ❯ test/all_functional.spec.ts > functional tests > ro_MD > location > state()
     → expected undefined to be truthy
   ❯ test/all_functional.spec.ts > functional tests > ro_MD > location > stateAbbr()
     → expected undefined to be truthy
   ❯ test/all_functional.spec.ts > functional tests > ro_MD > person > suffix()
     → expected undefined to be truthy
   ❯ test/all_functional.spec.ts > faker.helpers.fake functional tests > ro_MD > location > state()
     → expected 'undefined' not to be 'undefined' // Object.is equality
   ❯ test/all_functional.spec.ts > faker.helpers.fake functional tests > ro_MD > location > stateAbbr()
     → expected 'undefined' not to be 'undefined' // Object.is equality
   ❯ test/all_functional.spec.ts > faker.helpers.fake functional tests > ro_MD > person > suffix()
     → expected 'undefined' not to be 'undefined' // Object.is equality

?

@Shinigami92
Copy link
Member

I assume this test ensures that inside some locale files exporting empty arrays is not allowed
Sadly the test result message is not indicating this in any way

@ST-DDT please tell me if I'm right or wrong

@ST-DDT
Copy link
Member

ST-DDT commented Feb 21, 2023 •
edited
Loading

How should I fix this

For now please add them to the skipped locale entries (with a comment that this locale doesn't have that data).

I assume this test ensures that inside some locale files exporting empty arrays is not allowed
Sadly the test result message is not indicating this in any way

That is correct. It will do so when #893 is merged.

@botzill
Copy link
Author

botzill commented Feb 22, 2023

For now please add them to the skipped locale entries (with a comment that this locale doesn't have that data).

Hi, can you guid on how todo this?

@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Feb 22, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Feb 22, 2023

For now please add them to the skipped locale entries (with a comment that this locale doesn't have that data).

Hi, can you guid on how todo this?

Done. Thanks for your patience.

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Merging #1848 (91a0aa8) into next (25bd847) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff            @@
##             next    #1848     +/-   ##
=========================================
  Coverage   99.62%   99.63%             
=========================================
  Files        2355     2392     +37     
  Lines      236403   237965   +1562     
  Branches     1153     1158      +5     
=========================================
+ Hits       235527   237099   +1572     
+ Misses        854      844     -10     
  Partials       22       22
Impacted Files Coverage Δ
src/locale/ro_MD.ts 100.00% <100.00%> (ø)
src/locales/index.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/cell_phone/formats.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/cell_phone/index.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/date/index.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/date/month.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/date/weekday.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/index.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/internet/domain_suffix.ts 100.00% <100.00%> (ø)
src/locales/ro_MD/internet/free_email.ts 100.00% <100.00%> (ø)
... and 29 more

@ST-DDT ST-DDT requested review from matthewmayer, Shinigami92 and a team February 22, 2023 23:51
matthewmayer
matthewmayer requested changes Mar 14, 2023
View reviewed changes
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.

Now that #1340 has been closed we have support for multiple fallback languages, so it would make sense to change the fallback languages

ro_MD -> ro -> en, so ro_MD only includes MD-specific data.

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Mar 14, 2023
@botzill botzill closed this Mar 14, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Mar 14, 2023

@botzill Did you close this PR by accident?

@botzill
Copy link
Author

botzill commented Mar 14, 2023

hm, @ST-DDT I think yes,

@botzill
Copy link
Author

botzill commented Mar 14, 2023

How can I get it back?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 14, 2023 •
edited
Loading

The force push comment contains a reference to the old version: 91a0aa8

You should be able to locally checkout the commit or reset your branch to that commit.

@botzill
Copy link
Author

botzill commented Mar 15, 2023

Hi @ST-DDT, I do not have that comit on my end. I did rebase and I create a new PR, let me know if that helps: #1936

@xDivisionByZerox xDivisionByZerox mentioned this pull request Jul 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewmayer matthewmayer matthewmayer requested changes

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

@ST-DDT ST-DDT ST-DDT approved these changes

@Shinigami92 Shinigami92 Awaiting requested review from Shinigami92

Assignees

@botzill botzill

Labels
c: locale Permutes locale definitions needs rebase There is a merge conflict p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.x
Development

Successfully merging this pull request may close these issues.
5 participants
@botzill @matthewmayer @ST-DDT @Shinigami92 @xDivisionByZerox