Add supported countries tests #878
Conversation
- Add README.rst country count test - Add README.rst supported countries tests - Add `UK` back to the list of supported countries - Sort `HolidayBase::subdivisions` - Fix supported countries table formatting
| @@ -272,7 +272,7 @@ def CountryHoliday( | |||
| ) | |||
|
|
|||
|
|
|||
| def list_supported_countries() -> Dict[str, List[str]]: | |||
| def list_supported_countries(unique=False) -> Dict[str, List[str]]: | |||
There was a problem hiding this comment.
@arkid15r I am working on #998 and need to update this function, but stumbled on this unique=False addition which I find zero documentation about (not even in the docstring!).
Since it's undocumented the default is to drop it. Thoughts?
Also, a good reminder that nothing should be released (PR merged) unless it's thoroughly documented, and we're probably a bit behind in that front.
There was a problem hiding this comment.
It's always a good idea to search for existing references before deletion even if there is no documentation about something. I'd do it first in general and for sure in this particular case (as it's definitely in use).
If you allow, I generalize your reminder re. thorough documentation and lift it to another level: any PR should go through a thorough review. This PR had been sitting in the queue for about half a month before I merged it. It seems that I focused on tests improvements and missed the documentation part.
The good news is that the team has grown since that time so we are able to do regular reviews for [almost] every PR.
As for the fix -- I'll add the docstring, shouldn't be too hard to implement.
Thanks for bringing this up, Mike!
There was a problem hiding this comment.
It's always a good idea to search for existing references before deletion even if there is no documentation about something. I'd do it first in general and for sure in this particular case (as it's definitely in use).
Wise words.
If you allow, I generalize your reminder re. thorough documentation and lift it to another level: any PR should go through a thorough review. This PR had been sitting in the queue for about half a month before I merged it. It seems that I focused on tests improvements and missed the documentation part.
Wiser words.
As for the fix -- I'll add the docstring, shouldn't be too hard to implement.
Please wait. I found all references (all are in testing) and the unique argument may no longer be needed if I can get the lazy loading to work (stuck on one particular aspect at the moment).
Thanks for all the great work Arkadii!
| @@ -281,13 +281,13 @@ def list_supported_countries() -> Dict[str, List[str]]: | |||
| the value is a list of supported subdivision codes. | |||
| """ | |||
| return { | |||
| cls.country: cls.subdivisions | |||
| cls.country if unique else name: cls.subdivisions | |||
There was a problem hiding this comment.
@arkid15r this code generates a Set when unique is set, which does not conform to the declared return type.
There was a problem hiding this comment.
Couldn't reproduce that:
>>> import holidays
>>> holidays.__version__
'0.22'
>>> type(holidays.list_supported_countries(unique=True))
<class 'dict'>
| for name, cls in inspect.getmembers(countries, inspect.isclass) | ||
| if len(name) == 2 and issubclass(cls, HolidayBase) | ||
| } | ||
|
|
||
|
|
||
| def list_supported_financial() -> Dict[str, List[str]]: | ||
| def list_supported_financial(unique=False) -> Dict[str, List[str]]: |
| @@ -296,8 +296,8 @@ def list_supported_financial() -> Dict[str, List[str]]: | |||
| the value is a list of supported subdivision codes. | |||
| """ | |||
| return { | |||
| cls.market: cls.subdivisions | |||
| for _, cls in inspect.getmembers(financial, inspect.isclass) | |||
| cls.market if unique else name: cls.subdivisions | |||
UKback to the list of supported countriesHolidayBase::subdivisions