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

Date dictionaries are initially empty #1824

Closed
dsoprea opened this issue Jun 10, 2024 · 6 comments
Closed

Date dictionaries are initially empty #1824

dsoprea opened this issue Jun 10, 2024 · 6 comments
Assignees

Comments

@dsoprea
Copy link

dsoprea commented Jun 10, 2024

The documentation very states that "holidays.<country>()" returns a dictionary in multiple places:

https://github.com/vacanza/python-holidays/blob/fde28db1d17e05e0853585936b8b8f85e9e38d34/README.rst?plain=1#L106

This is misleading. It's a dictionary-like object.

It's also initially empty, which is unexpected when you just refer to it as a dictionary:

>>> x = holidays.US()

>>> dict(x)
{}

>>> x['2024-07-04']
'Independence Day'

>>> dict(x)
{datetime.date(2024, 1, 1): "New Year's Day", datetime.date(2024, 5, 27): 'Memorial Day', datetime.date(2024, 6, 19): 'Juneteenth National Independence Day', datetime.date(2024, 7, 4): 'Independence Day', datetime.date(2024, 9, 2): 'Labor Day', datetime.date(2024, 11, 11): 'Veterans Day', datetime.date(2024, 11, 28): 'Thanksgiving', datetime.date(2024, 12, 25): 'Christmas Day', datetime.date(2024, 1, 15): 'Martin Luther King Jr. Day', datetime.date(2024, 2, 19): "Washington's Birthday", datetime.date(2024, 10, 14): 'Columbus Day'}

Here, we had to do a membership check first (either via indexing or get()). Population appears to be lazy-loaded and isn't triggered by just printing the dictionary (__dict__).

If you don't want to change/fix this, then at least don't aggressively imply that the data will be there and ready as soon as you instantiate the class. It's confusing and this proper handling is not documented.

@arkid15r
Copy link
Collaborator

Hi @dsoprea
Thanks for bringing this up!

This is misleading. It's a dictionary-like object.

I agree w/ you on this. It's 100% dict-like object.

If you don't want to change/fix this, then at least don't aggressively imply that the data will be there and ready as soon as you instantiate the class. It's confusing and this proper handling is not documented.

However this one is something I have a different angle on. Could you specify what made you think that the dictionary would be populated w/ holidays? Originally holidays package was designed w/ performance in mind. Having said that I don't think that behavior is going to be changed/fixed.

I'm mostly interested on documentation improvement related to this part:

at least don't aggressively imply that the data will be there and ready as soon as you instantiate the class.

Thanks for your help!

@arkid15r arkid15r self-assigned this Jun 10, 2024
@dsoprea
Copy link
Author

dsoprea commented Jun 10, 2024

A dictionary is thought of as a static type (a static one not an immutable one, to be clear). By nature of returning a dictionary, you assume that there are no other steps to be taken for it to have data. Especially when the dataset is already specific and thought of as being fairly static. If you tell me that you're going to essentially give me a "dictionary of holidays" but what you give me will always be an empty dictionary of holidays due to undocumented behavior then I'd consider that a broken design.

I don't think we're thinking of performance the right way. You've already overloaded the concept of dictionaries and are doing some lazy-loading. If, however, the developer is calling keys() or values() (or copying/materializing via dict()), they are clearly signaling the library to lazy-load, and the library currently does nothing. Currently, they need to hardcode a lookup of a specific holiday, even though they may actually want all of them, to get things moving because the library doesn't actually support it right after initialization. That's also broken, to me. There is an innate understanding that if I ask for all of the holidays then I am willing to wait for it.

@arkid15r
Copy link
Collaborator

I thought we both agreed it's not a dict but a dict-like object. It seem that your whole case is built upon this misleading comment that needs to be fixed.

Also I beg to disagree on your "undocumented behavior" comment.

I believe your ideas would be helpful to consider while implementing holidays v1. Maybe even v0 could use some sort of eager population flag (w/o changing the default behavior).

As you can see this project could use external contributor's help. Please let us know if you're willing to help with that.

@arkid15r arkid15r mentioned this issue Jun 14, 2024
10 tasks
@KJhellico
Copy link
Collaborator

It's also initially empty, which is unexpected when you just refer to it as a dictionary:

>>> x = holidays.US()

>>> dict(x)
{}

You can just initialize holidays object with required years interval when creating it:
x = holidays.US(years=2024)
or
x = holidays.US(years=range(2000, 2025))
and it will contain required values.

@dsoprea
Copy link
Author

dsoprea commented Jun 15, 2024

@KJhellico Thanks. That's really useful.

@arkid15r
Copy link
Collaborator

Marking this as resolved.

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

No branches or pull requests

3 participants