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

Add language code definitions and test #141

Merged
merged 2 commits into from
Feb 26, 2024
Merged

Conversation

07pepa
Copy link
Contributor

@07pepa 07pepa commented Feb 23, 2024

I added language definitions

@07pepa 07pepa force-pushed the language_code branch 2 times, most recently from 6cc8b7d to 30c54c4 Compare February 23, 2024 15:46
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e186814) to head (e30ac0a).
Report is 4 commits behind head on main.

❗ Current head e30ac0a differs from pull request most recent head 1fff4e3. Consider uploading reports for the commit 1fff4e3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #141   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        12    +1     
  Lines          685       724   +39     
  Branches       169       179   +10     
=========================================
+ Hits           685       724   +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@07pepa 07pepa force-pushed the language_code branch 2 times, most recently from 393f4e9 to 1921942 Compare February 23, 2024 15:58
@yezz123 yezz123 self-requested a review February 23, 2024 21:31
@yezz123 yezz123 self-assigned this Feb 23, 2024
@yezz123
Copy link
Collaborator

yezz123 commented Feb 23, 2024

Hello @07pepa That's a great start for the PR, but we should now add Pydantic CoreSchema & the validation - check here an example, also including a test for JSON schema - check here

@07pepa 07pepa force-pushed the language_code branch 6 times, most recently from b2043b1 to fb8ebf8 Compare February 24, 2024 10:31
@07pepa
Copy link
Contributor Author

07pepa commented Feb 24, 2024

Hello @07pepa That's a great start for the PR, but we should now add Pydantic CoreSchema & the validation - check here an example, also including a test for JSON schema - check here

I am not sure how/why to add this to literal (since it is supported by pydantic)
also litteral is recomended for "enumerations" https://docs.pydantic.dev/latest/concepts/performance/#use-literal-not-enum

I am not sure what you want me to do. please tell me how to do it or just modify the code.. i am fine with that.

Only thing i would add now is a check if all codes are supported with help of some online source of truth (but that would complicate the test code considerably. and we would have to decide on source of truth

@07pepa 07pepa force-pushed the language_code branch 3 times, most recently from 3336049 to 8cf42e1 Compare February 24, 2024 11:15
@yezz123
Copy link
Collaborator

yezz123 commented Feb 24, 2024

Hello @07pepa,

Thank you for your contribution! This PR is indeed valuable, particularly in terms of structuring the data. However, I'd like to propose an enhancement, we can use pycountry for not only countries but also languages, this ensuring consistency and using existing resources already.

As we're already utilizing pycountry for country data, integrating language support from pycountry seems like a natural extension. By doing so, we can ensure uniformity and benefit from the robustness of pycountry's language code support.

Here are some initial resources to help kickstart this integration:

Once we establish a similar structure as the one we have for countries in country.py, we can seamlessly support language codes as well. This enhancement would be highly beneficial. Thank you for considering it! 🙏🏻

@07pepa
Copy link
Contributor Author

07pepa commented Feb 24, 2024

good point i will look in detail on monday

@07pepa 07pepa force-pushed the language_code branch 8 times, most recently from 709f459 to 0700867 Compare February 26, 2024 13:01
@07pepa 07pepa force-pushed the language_code branch 2 times, most recently from 5a0a268 to ca6ac2a Compare February 26, 2024 15:16
* added dynamically generated literals based on pycountry
* tested all possibilities and errors exhaustively
@07pepa
Copy link
Contributor Author

07pepa commented Feb 26, 2024

@yezz123 i am finished .... and test are passing (test are exhaustive)

can you add your input on this code or just merge it .... i will probably add curecies next

Copy link
Collaborator

@yezz123 yezz123 left a comment

Choose a reason for hiding this comment

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

LGTM ⭐ Thank you @07pepa 🙏🏻

I've added documentation to each function and class, and I will open a PR to add it to the documentation as soon as we release a new version.

@yezz123 yezz123 merged commit b10bb90 into pydantic:main Feb 26, 2024
18 checks passed
@07pepa 07pepa deleted the language_code branch May 12, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for ISO 639 (639-1, 639-2/T, 639-2/B) codes used to classify languages
2 participants