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 locale TH #1921

Closed
wants to merge 5 commits into from
Closed

Conversation

munkeawtoast
Copy link
Contributor

No description provided.

@munkeawtoast munkeawtoast requested a review from a team March 11, 2023 21:54
@munkeawtoast munkeawtoast requested a review from a team as a code owner March 11, 2023 21:54
@ejcheng ejcheng added p: 1-normal Nothing urgent c: locale Permutes locale definitions labels Mar 12, 2023
@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

Merging #1921 (868aed2) into next (8f0abd3) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 868aed2 differs from pull request most recent head d3dfec4. Consider uploading reports for the commit d3dfec4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1921      +/-   ##
==========================================
+ Coverage   99.62%   99.63%   +0.01%     
==========================================
  Files        2357     2384      +27     
  Lines      236564   238369    +1805     
  Branches     1192     1198       +6     
==========================================
+ Hits       235675   237507    +1832     
+ Misses        867      840      -27     
  Partials       22       22              
Impacted Files Coverage Δ
src/locale/index.ts 100.00% <100.00%> (ø)
src/locale/th.ts 100.00% <100.00%> (ø)
src/locales/index.ts 100.00% <100.00%> (ø)
src/locales/th/animal/cat.ts 100.00% <100.00%> (ø)
src/locales/th/animal/dog.ts 100.00% <100.00%> (ø)
src/locales/th/animal/index.ts 100.00% <100.00%> (ø)
src/locales/th/cell_phone/formats.ts 100.00% <100.00%> (ø)
src/locales/th/cell_phone/index.ts 100.00% <100.00%> (ø)
src/locales/th/color/human.ts 100.00% <100.00%> (ø)
src/locales/th/color/index.ts 100.00% <100.00%> (ø)
... and 19 more

... and 2 files with indirect coverage 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.

ขอบคุณ!

Left a few comments

@@ -118,6 +118,7 @@ export const customFaker = new Faker({
| `ru` | Russian | `fakerRU` |
| `sk` | Slovakian | `fakerSK` |
| `sv` | Swedish | `fakerSV` |
| `th` | TODO: Insert Title for th | `fakerTH` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to "Thai".

import person from './person';

const th: LocaleDefinition = {
title: 'TODO: Insert Title for th',
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to Thai

@@ -0,0 +1,172 @@
export default [
'คมคาย',
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that there aren't really any common Thai surnames, so how did you select these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are mostly generated from chatGPT. I took time to filter out the unnatural ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

that might be problematic, because we need data to be MIT licensed and the output of ChatGPT could contain copyrighted material https://github.com/faker-js/faker/blob/next/CONTRIBUTING.md#sourcing-data-for-definitions

An alternative might be to make some fake Thai surnames by taking syllables which commonly appear in Thai names and joining them together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try it out!

import male_first_name from './male_first_name';

const person: PersonDefinitions = {
female_first_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

you should also add a src/locales/th/person/name.ts to give the correct name order for Thai, ie

export default [
  { value: '{{person.firstName}} {{person.lastName}}', weight: 1 },
];

@@ -0,0 +1 @@
export default ['06 #### ####', '08 #### ####', '09 #### ####'];
Copy link
Contributor

Choose a reason for hiding this comment

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

this file is deprecated, you can also add src/locales/th/phone_number/formats.ts as well

@munkeawtoast munkeawtoast deleted the branch faker-js:next March 12, 2023 16:00
@munkeawtoast munkeawtoast deleted the next branch March 12, 2023 16:00
@munkeawtoast
Copy link
Contributor Author

Oops i renamed the branch and didn't think it would close the PR. Should i make a new PR?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 12, 2023

If it is easier for you, then yes. But please make sure to fix all comments posted here.

@munkeawtoast munkeawtoast restored the next branch March 12, 2023 18:17
@munkeawtoast munkeawtoast deleted the next branch March 12, 2023 18:17
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
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants