python-holidays icon indicating copy to clipboard operation
python-holidays copied to clipboard

Change HolidayBase subdivisions type from `List[str]` to `Tuple[str, ...]`.

Open arkid15r opened this issue 1 year ago • 1 comments

Also make the same change for _deprecated_subdivisions.

As a result the HolidaySum internal attributes are also tuples instead of lists now (country, market, subdiv).

The list_supported_countries() and list_supported_financial() from utils.py now return Dict[str, Tuple[str]] (was Dict[str, Tuple[str]]).

Sort countries subdivisions alphabetically. Remove empty subdivisions.

arkid15r avatar Oct 04 '22 16:10 arkid15r

Coverage Status

Coverage remained the same at 100.0% when pulling d8f1e5289387dd6f9930b03450dcae938898f2e4 on arkid15r:subdiv-refactoring into d58316934ca7b314af10c601ca72d2cabd82c34e on dr-prodigy:beta.

coveralls avatar Oct 04 '22 17:10 coveralls

Hi @arkid15r thank you!

Although I understand that, being read-only, tuples would be more appropriate to retain subdivision data, we should always aim to keep compatibility with our previous versions of the lib (a very large number of different sw depends on python-holidays), so if it's not really required, I would rather avoid applying this breaking change. Is there any other technical reason to release it?

Pls let me know, thank you very much!

dr-prodigy avatar Oct 18 '22 06:10 dr-prodigy

@dr-prodigy Quick question (mostly for self-education), but what kind of break do you envision? Both lists and tuples are Iterables, so I can't think of any, but have less experience with this library. I see as an argument in favor of adopting this PR the fact that in general (as I understand it) tuples are more efficient users of memory and generally faster to process, so it's in complete alignment with the core tenet of this library ("A fast, efficient Python library...").

mborsetti avatar Oct 19 '22 19:10 mborsetti

@dr-prodigy Quick question (mostly for self-education), but what kind of break do you envision? Both lists and tuples are Iterables, so I can't think of any, but have less experience with this library. I see as an argument in favor of adopting this PR the fact that in general (as I understand it) tuples are more efficient users of memory and generally faster to process, so it's in complete alignment with the core tenet of this library ("A fast, efficient Python library...").

Hey Mike! While your considerations are undoubtly correct:

  • even if both are Iterables (which could most probably cover 99.9% of the cases), Lists and Tuples are different objects anyway, so a code assuming to receive a List of subdivisions (not very clever.. but we've seen something worse in the past :-) ) would break anyway
  • also, the improved efficiency in memory usage and speed is really (really!) negligible (we're talking about lists of just a few elements, which are also never modified)

In the end, you are right, there are no big issues, but not even very good reasons to apply this change.. just that .. hope this makes some sense to you too :-)

dr-prodigy avatar Nov 06 '22 21:11 dr-prodigy

@dr-prodigy Thanks for sharing your knowledge!

mborsetti avatar Nov 06 '22 22:11 mborsetti

Mauri, Mike, thanks for your inputs!

I understand the compatibility concern here. It makes sense to keep externally consumed results as lists for any (weird or not) use cases. I added respective changes to HolidaySum, utils.list_supported_countries and utils.list_supported_financial files. They have original types (list) now.

In my opinion it also does make sense to switch to tuples in internal data containers (e.g. subdivisions of HolidayBase). For better compatibility I kept the old type for the subdivisions and just combined it with the new Tuple approach so both kinds are available now (just in case). From my perspective this attribute describes a country's specific structure and should never be modified directly (haven't found any python-holidays documentation examples as well). And even in case it was there shouldn't be an issue as we don't use any list specific methods.

I feel the compatibility/improvement balance is pretty good here.

@dr-prodigy if this PR still doesn't look convincing to you feel free to discard it. Otherwise I'd be happy to answer any questions related to the changes I'm trying to introduce.

Again, thanks for meaningful opinions and a friendly work environment!

arkid15r avatar Nov 07 '22 23:11 arkid15r