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

Adjust for and require icalendar>=6.0.0 #1364

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

iamleot
Copy link

@iamleot iamleot commented Oct 31, 2024

This PR makes khal compatible with icalendar 6.0.0 and newer (and start to require it!).

In particular:

  • For timezone functionalities icalendar can use both pytz and zoneinfo, preferring zoneinfo. We rely on pytz for several attributes, stick to it. This fixes Support icalendar v6 #1361 and should obsolete Replace tz.localize with date.replace #1363.
  • icalendar.windows_to_olson is no longer available, we need to refer to it as icalendar.timezone.windows_to_olson

With this PR all the tests passes again with icalendar>=6.0.0.

@iamleot
Copy link
Author

iamleot commented Oct 31, 2024

Leaving it as a draft because I need to RTFS of icalendar because maybe we need bump the version requirement and/or make it agnostic both to old and new icalendar.

use_pytz() appeared in icalendar-6.0.0 I have bumped the version requirement as well. Also icalendar.windows_to_olson is now exposed via icalendar.timezone.windows_to_olson from 6.0.0. If you think it is better to make the patch agnostic I think we can check if that function is available or not and I can try to adjust that too.

@iamleot iamleot force-pushed the icalendar-use-pytz branch from 3f42fc4 to d38921d Compare October 31, 2024 12:27
For timezone functionalities icalendar can use both pytz and zoneinfo,
preferring zoneinfo.

We rely on pytz for several attributes, stick to it.

Fixes pimutils#1361.
Should obsolete pimutils#1363.
Per "Hacking" document.
@iamleot iamleot force-pushed the icalendar-use-pytz branch from d38921d to 31884be Compare October 31, 2024 12:30
In icalendar-6.0.0 windows_to_olson is part of icalendar.timezone.
@iamleot iamleot marked this pull request as ready for review October 31, 2024 13:22
@iamleot iamleot changed the title Force usage of pytz Adjust for and require icalendar>=6.0.0 Oct 31, 2024
@iamleot iamleot changed the title Adjust for and require icalendar>=6.0.0 Adjust for and require icalendar>=6.0.0 Oct 31, 2024
@iamleot
Copy link
Author

iamleot commented Oct 31, 2024

The «docs/readthedocs.org:khal — Read the Docs build failed!» failures are probably unrelated to that. That's also tracked via #1356.

Copy link
Collaborator

@d7415 d7415 left a comment

Choose a reason for hiding this comment

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

icalendar 6.0.0 was only released at the end of September, so may not have propagated as far as we'd like.

Other than that, I like it.

@iamleot
Copy link
Author

iamleot commented Oct 31, 2024

icalendar 6.0.0 was only released at the end of September, so may not have propagated as far as we'd like.

Mmm, that's a good point.

Maybe we can try to do some dance as follows...

For use_pytz() we can add:

try:
    icalendar.use_pytz()
except AttributeError:
    # assumes icalendar<6.0.0, only pytz is used
    pass

While for icalendar.windows_to_olson vs icalendar.timezone.windows_to_olson we can do:

try:
    from icalendar.timezone import windows_to_olson
except ImportError:
    from icalendar import windows_to_olson

...and then replace the icalendar.windows_to_olson to just windows_to_olson.

(Beware, both only written as comment, not really tested!)

But, in particular the latter... IMHO is not very nice!

However, if you think it's worth to be more version agnostic I'm happy to adjust the patch as described in this comment!

Other than that, I like it.

Thank you!

@d7415
Copy link
Collaborator

d7415 commented Oct 31, 2024

Those could certainly work. Remembering to take them about later would be a pain.

There's nothing strictly wrong with "New khal needs icalendar >=6. If you don't have it stick with the last version of khal"

I think it's best to see if Christian or Hugo have an opinion on this (deliberately not tagging them for now)

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Nov 6, 2024
This was submitted upstream but hasn't been merged yet because they are
trying to figure out how to maintain backwards compatibility with older
py3-icalendar versions.

pimutils/khal#1364
@iamleot
Copy link
Author

iamleot commented Nov 30, 2024

Hello folks!
We got a couple of other people hitting that.

Can I help in any way to get that possibly merged and/or if you think it's better to make it much more generic and agnostic as shared in the comments I can try to adjust that PR too!

Thank you!

@iamleot
Copy link
Author

iamleot commented Nov 30, 2024

(Also, if I should rebase that to latest main and then push with the force, please let me know too!)

[Right now I have not seen any significant conflicts but it can be helpful to re-validate that too against latest main!]

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.

Support icalendar v6
2 participants