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

Adding support for Tamil language #1378

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Adding support for Tamil language #1378

merged 2 commits into from
Oct 17, 2024

Conversation

sann3
Copy link
Contributor

@sann3 sann3 commented Oct 14, 2024

PR to fix #1377

Copy link

what-the-diff bot commented Oct 14, 2024

PR Summary

  • Introduction of Tamil Language Support
    We've added support for the Tamil language in our system, now listed as a supported locale in the README.md file.
  • New Tamil Language Data File
    A new file named ta.yml has been created. This file is essentially a detailed portfolio of Tamil language data. It includes the specifics for various fields such as names, addresses, companies, and phone number formats. This will make our system more accessible to Tamil-speaking users.

Copy link
Collaborator

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

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

@sann3 Thank you for the PR. Generally looks good, but I would ask for few improvements.

  1. Typo in commit message: "Create confiog for Tamil language"
  2. "ta" is a language used in multiple countries. While "+91" is India country code.
    So the "phone" section should be in file src/main/resources/_IN.yml. Phone is related to country, not language.

@kingthorin
Copy link
Collaborator

05:50:20.864 [ERROR] LocalityTest.allSupportedLocales:35 [Somebody forgot to add the new locale to Locality.LOCALES]

@sann3 sann3 force-pushed the main branch 2 times, most recently from df8f416 to 150329c Compare October 15, 2024 12:37
@sann3
Copy link
Contributor Author

sann3 commented Oct 15, 2024

Updated the PR with recommended changes, pls check.

@kingthorin
Copy link
Collaborator

@asolntsev

2. "ta" is a language used in multiple countries. While "+91" is India country code.
   So the "phone" section should be in file `src/main/resources/_IN.yml`. Phone is related to country, not language.

Same for address?

@asolntsev
Copy link
Collaborator

address

Not exactly.
For example, two-letter state codes should be in country gile, but state name, city names etc. should be in the language file because they sound differently in different languages.

@asolntsev
Copy link
Collaborator

@sann3 Please execute mvn test, and you will see some failing tests

mvn test
FakerIntegrationTest.testAllFakerMethodsThatReturnStrings:100->testAllMethodsThatReturnStringsActuallyReturnStrings:122 Runtime Test for method public java.lang.String net.datafaker.providers.base.Name.fullName() and object Name(Faker@955ae44)@481a64f2 was failed for locale ta [thread: FakerIntegrationTest.testAllFakerMethodsThatReturnStrings#20438#]

The reason is that this variable is not defined in ta.yml:

         - "#{female_last_name}"

@bodiam
Copy link
Contributor

bodiam commented Oct 16, 2024

Hi @sann3 , looks great, thanks. Few things:

  • The file needs to be called ta-IN.yml.
  • In the file, please change the name: field to this:
      name:
        - "#{male_first_name}"
        - "#{female_first_name}"

When both items have been done, we can merge the PR!

@asolntsev
Copy link
Collaborator

  • The file needs to be called ta-IN.yml.

Not exactly.

  • Language-specific parts in ta.yml, and
  • country-specific parts in _IN.yml

See last section in CONTRIBUTING.md

@sann3
Copy link
Contributor Author

sann3 commented Oct 16, 2024

FakerIntegrationTest.testAllFakerMethodsThatReturnStrings test is failing with " Error type: NOT_A_NUMBER. The string supplied did not seem to be a phone number."

If I rename to ta-IN.yml, all the tests are passing. What should I do ?

@asolntsev asolntsev self-assigned this Oct 17, 2024
@asolntsev asolntsev merged commit 61af169 into datafaker-net:main Oct 17, 2024
1 of 10 checks passed
asolntsev added a commit that referenced this pull request Oct 17, 2024
When users call `new Faker(new Locale("ta")).phone()`, we create phone number for India (though Tamil is used in multiple countries).
asolntsev added a commit that referenced this pull request Oct 17, 2024
…age description

These settings already existed in "_IN.yml" (India country specific properties).
asolntsev added a commit that referenced this pull request Oct 17, 2024
…ta-IN.yml"

When users call `new Faker(new Locale("ta")).phone()`, we create phone number for India (though Tamil is used in multiple countries).
@asolntsev
Copy link
Collaborator

@sann3 All good, I've merged your PR and made few changes in a new PR https://github.com/datafaker-net/datafaker/pull/1385/commits

That was not trivial because of some legacy and hacks in DF code. :)

asolntsev added a commit that referenced this pull request Oct 17, 2024
When users call `new Faker(new Locale("ta")).phone()`, we create phone number for India (though Tamil is used in multiple countries).
asolntsev added a commit that referenced this pull request Oct 17, 2024
…age description

These settings already existed in "_IN.yml" (India country specific properties).
asolntsev added a commit that referenced this pull request Oct 17, 2024
…ta-IN.yml"

When users call `new Faker(new Locale("ta")).phone()`, we create phone number for India (though Tamil is used in multiple countries).
@sann3
Copy link
Contributor Author

sann3 commented Oct 24, 2024

Can someone pleas add hacktoberfest-accepted label for this pr

@asolntsev
Copy link
Collaborator

Em... why? This PR is already merged.

@sann3
Copy link
Contributor Author

sann3 commented Oct 24, 2024

So that it will be counted against the my hacktoberfest Pr count.

@asolntsev
Copy link
Collaborator

ah, I got it. Added the label. :)

@sann3
Copy link
Contributor Author

sann3 commented Oct 24, 2024

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for tamil language
4 participants