-
Notifications
You must be signed in to change notification settings - Fork 466
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 ISO 3166-1 for overseas departments and regions of France #1063
base: dev
Are you sure you want to change the base?
Conversation
I'd like to localize these countries using the France localization (without creating separate files for each country), but I don't know how to do it. :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good! However, I have a doubt regarding the whole idea of adding regions/territories/etc (not a country per se) as top level entities. I would definitely like to hear other points of view on this. So I'm skipping this PR for now until we decide what our ultimate supported countries list looks like (UN members, all alpha-2 entities, other?).
I'd definitely accept all the tests thought :)
country = "GF" | ||
subdivisions = [] | ||
|
||
def _populate(self, year: int) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect it to be done via init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
country
and subdivisions
? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea :)
The correct selection should have been the whole populate method (ln. 23-25). My goal was to suggest using init instead of populate as it doesn't seem you're doing actual populate method customization.
My thought is that if a country, sovereign state, or dependent territory qualifies under the rules of the International Organization for Standardization, which consists of 167 countries, to have its own code and therefore for the Internet Assigned Numbers Authority to assign it a unique country code top-level domain (ccTLD), then this library should provide holidays for it. More pragmatically, I don't think that it would be good for this project to be involved in the political activity of arbitrating which country, sovereign state, or dependent territory gets to have coverage here and which one does not, with the messiness of coming up with new rules, and in effect creating a non-standard classification apart from the already well-established ISO/IANA one. In my own opinion sticking to the global and widely recognized standards of ISO and IANA is the fairest and most expedient way to provide holiday coverage, as well as the most useful to developers. @arkid15r I urge you to accept this PR and push it once ready. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Proposed change
This PR implements ISO 3166-1 country entries for overseas departments and regions of France that have officially assigned their own country codes:
Type of change
Checklist
beta
branch of the repositorymake pre-commit
)make test
,make tox
)