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(locale): add Serbian (Latin) language #1801

Merged
merged 21 commits into from
Mar 20, 2023

Conversation

bdimitrovski
Copy link
Contributor

No description provided.

@import-brain import-brain added p: 1-normal Nothing urgent c: locale Permutes locale definitions labels Jan 31, 2023
Copy link
Member

@import-brain import-brain left a comment

Choose a reason for hiding this comment

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

Please remove the duplicated values in your locale data as shown here

https://github.com/faker-js/faker/actions/runs/4050174605/jobs/6967813401

@bdimitrovski
Copy link
Contributor Author

@import-brain Done, the tests should be passing now.

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #1801 (93e123f) into next (93e1433) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 93e123f differs from pull request most recent head 6a5c268. Consider uploading reports for the commit 6a5c268 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1801      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2354     2385      +31     
  Lines      236555   239261    +2706     
  Branches     1227     1226       -1     
==========================================
+ Hits       235673   238363    +2690     
- Misses        860      876      +16     
  Partials       22       22              
Impacted Files Coverage Δ
src/locale/index.ts 100.00% <100.00%> (ø)
src/locale/sr_RS_latin.ts 100.00% <100.00%> (ø)
src/locales/index.ts 100.00% <100.00%> (ø)
src/locales/sr_RS_latin/cell_phone/formats.ts 100.00% <100.00%> (ø)
src/locales/sr_RS_latin/cell_phone/index.ts 100.00% <100.00%> (ø)
src/locales/sr_RS_latin/date/index.ts 100.00% <100.00%> (ø)
src/locales/sr_RS_latin/date/month.ts 100.00% <100.00%> (ø)
src/locales/sr_RS_latin/date/weekday.ts 100.00% <100.00%> (ø)
src/locales/sr_RS_latin/index.ts 100.00% <100.00%> (ø)
src/locales/sr_RS_latin/internet/domain_suffix.ts 100.00% <100.00%> (ø)
... and 20 more

... and 11 files with indirect coverage changes

src/locales/sr_RS/date/month.ts Outdated Show resolved Hide resolved
src/locales/sr_RS/date/month.ts Outdated Show resolved Hide resolved
src/locales/sr_RS/date/weekday.ts Outdated Show resolved Hide resolved
src/locales/sr_RS/date/weekday.ts Outdated Show resolved Hide resolved
src/locales/sr_RS/person/last_name.ts Outdated Show resolved Hide resolved
src/locales/sr_RS/person/name.ts Outdated Show resolved Hide resolved
@bdimitrovski
Copy link
Contributor Author

@matthewmayer @ST-DDT when could we expect this to land?

@matthewmayer
Copy link
Contributor

Probably best to use your own fork if you are relying on its functionality in the short term.

https://blaipratdesaba.com/how-to-use-an-npm-node-module-that-has-been-forked-b7dd522fdd08

Even when it lands it will only be going into 8.0.0 which is still in alpha.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 9, 2023

We are currently busy and thus are a little bit slower with the reviews.
AFAIK neither of us has any Serbian skills, so it takes a bit of time for us to review.
We will review/merge it when we have time, probably soon.
You can speed up the process by getting reviews from a third-party.

As for a release, we might release another v8 alpha in the next month (not sure yet),
but the actual v8 stable release will take at least one or two months.


Also I noticed, that there are two "Serbian" languages, that seem to have the same locale code.

sr-RS@latin + sr-RS (Russian characters + some more?)

https://github.com/validatorjs/validator.js/blob/master/src/lib/alpha.js#L24-L25

I'm not sure what the correct locale code would be here.
Can you give us some insides on this issue? Should we use sr_RS_latin as locale code for clarification?

@bdimitrovski
Copy link
Contributor Author

We are currently busy and thus are a little bit slower with the reviews. AFAIK neither of us has any Serbian skills, so it takes a bit of time for us to review. We will review/merge it when we have time, probably soon. You can speed up the process by getting reviews from a third-party.

As for a release, we might release another v8 alpha in the next month (not sure yet), but the actual v8 stable release will take at least one or two months.

Also I noticed, that there are two "Serbian" languages, that seem to have the same locale code.

sr-RS@latin + sr-RS (Russian characters + some more?)

https://github.com/validatorjs/validator.js/blob/master/src/lib/alpha.js#L24-L25

I'm not sure what the correct locale code would be here. Can you give us some insides on this issue? Should we use sr_RS_latin as locale code for clarification?

That's alright, I understand that you're busy, however, these are mostly just Serbian names, cities and some addresses, nothing fancy.

You can use the sr_RS_latin code, as we also have the Cyrilic alphabet, however, I have not translated it on this occasion. I can add it in another iteration, however, Latin would be more of a priority at this point.

@matthewmayer
Copy link
Contributor

seems sr_rs_latn and sr_rs_cyrl (or sr_latn_rs,sr_latn_cyrl) are more common in various software which distinguishes the two

@matthewmayer
Copy link
Contributor

bit of background https://ciklopea.com/blog/localization/should-you-localize-to-serbian-latin-or-to-serbian-cyrillic/

@bdimitrovski one thing i wasn't totally clear on: can you do a perfect 1-1 transliteration between the Latin and Cyrillic scripts? ie a computer should be able to translate character by characters? Or does it require some context/knowledge

@ST-DDT
Copy link
Member

ST-DDT commented Feb 11, 2023

seems sr_rs_latn and sr_rs_cyrl (or sr_latn_rs,sr_latn_cyrl) are more common in various software which distinguishes the two

Do you have a source for this?

My search results show otherwise:

https://www.google.com/search?q=%22sr_RS_Latin%22 => 747.000
https://www.google.com/search?q=%22sr_RS_Latn%22 => 55
https://www.google.com/search?q=%22sr-RS-Latin%22 => 5260
https://www.google.com/search?q=%22sr-RS-Latn%22 => 1040

Also I found this entry from the rust sources:

https://docs.rs/pure-rust-locales/0.2.1/x86_64-apple-darwin/pure_rust_locales/sr_RS_latin/LC_TIME/index.html

@ST-DDT
Copy link
Member

ST-DDT commented Mar 7, 2023

This PR needs to be updated:

git fetch --all
git merge origin/next
pnpm run preflight
git add .
git merge --continue

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Mar 7, 2023
@Shinigami92
Copy link
Member

Should we merge this as is or should we rename it to sr_RS_latin?

A little bit of googling showed that sr_RS_latin is not a bad idea and theoretically "valid"
If another one also says yes, lets do it 👍

@bdimitrovski
Copy link
Contributor Author

@matthewmayer @ST-DDT @Shinigami92 is this ready to go then?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 16, 2023

We will discuss the locale key tomorrow in our team meeting.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Mar 16, 2023
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.

Team Decision

docs.rs/pure-rust-locales/0.2.1/x86_64-apple-darwin/pure_rust_locales/sr_RS_latin/LC_TIME/index.html

Based on that source we have decided to use sr_RS_latin as locale key.
Please change the locale key accordingly.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Mar 16, 2023
@bdimitrovski
Copy link
Contributor Author

sr_RS_latin

@ST-DDT How/where would I change that? Which file(s) should be altered?

@matthewmayer
Copy link
Contributor

matthewmayer commented Mar 17, 2023

Basically

  1. rename the whole src/locales/sr_RS to src/locales/sr_RS_latin
  2. do a global text replace from sr_RS to sr_RS_latin
  3. run pnpm run preflight to make sure everything's OK

@bdimitrovski
Copy link
Contributor Author

bdimitrovski commented Mar 17, 2023

Basically

  1. rename the whole src/locales/sr_RS to src/locales/sr_RS_latin
  2. do a global text replace from sr_RS to sr_RS_latin
  3. run pnpm run preflight to make sure everything's OK

@matthewmayer Sure thing, that's what my thinking was too. But should I do that even in automatically generated files, like these?

Screenshot 2023-03-17 at 11 40 11

At this point, the sr_RS does not exist anymore, since I've renamed it, but when I run pnpm run preflight it still generates that.

@matthewmayer
Copy link
Contributor

Basically

  1. rename the whole src/locales/sr_RS to src/locales/sr_RS_latin
  2. do a global text replace from sr_RS to sr_RS_latin
  3. run pnpm run preflight to make sure everything's OK

@matthewmayer Sure thing, that's what my thinking was too. But should I do that even in automatically generated files, like these?

Screenshot 2023-03-17 at 11 40 11

doesn't really matter, they'll get regenerated by pnpm run preflight anyway.

@bdimitrovski
Copy link
Contributor Author

@matthewmayer @ST-DDT all done.

src/locale/sr_RS.ts Outdated Show resolved Hide resolved
@bdimitrovski
Copy link
Contributor Author

bit of background https://ciklopea.com/blog/localization/should-you-localize-to-serbian-latin-or-to-serbian-cyrillic/

@bdimitrovski one thing i wasn't totally clear on: can you do a perfect 1-1 transliteration between the Latin and Cyrillic scripts? ie a computer should be able to translate character by characters? Or does it require some context/knowledge

Sorry, I've only seen this now - as for the transliteration, yes, it is possible - you can see it in action here https://www.lexilogos.com/keyboard/serbian_conversion.htm

Serbian language is the only European language to use a 2 way lettering system (otherwise known as full synchronic digraphia), the Cyrillic letter is in formal use (government, documents, etc), while informally, almost everyone uses Latin, in everyday communication.

Automatic transliteration to Cyrillic of the work that I've done for the Latin letter, is possible (again, referring to e.g. lexilogos.com):

Screenshot 2023-03-17 at 14 41 30

@ST-DDT ST-DDT enabled auto-merge (squash) March 20, 2023 18:47
@ST-DDT ST-DDT merged commit d2046e8 into faker-js:next Mar 20, 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 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.

None yet

5 participants