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

Handle pytz timezones in iCalendar serialization #33

Merged
merged 1 commit into from
Aug 26, 2016

Conversation

medmunds
Copy link
Contributor

If pytz is installed, deal with its potential
NonExistentTimeError and AmbiguousTimeError
when serializing a VTIMEZONE.

Fixes #32

@medmunds
Copy link
Contributor Author

The test case runs only if pytz is installed. I don't really know a good way to handle that with Travis. (Well, other than adding with/without pytz to the Travis matrix, so that every platform runs twice. Let me know if you'd like me to look at that.)

Also, the pytz import in icalendar.py got a little more verbose than I'd like, to keep quantifiedcode happy. If there's some comment pragma to tell QC to ignore a line (like flake8's # NOQA), I can clean that up a bit. (I couldn't find one for QC.)

If pytz is installed, deal with its potential
NonExistentTimeError and AmbiguousTimeError
when serializing a VTIMEZONE.

Fixes skarim#32
@medmunds
Copy link
Contributor Author

Updated to clear the tzid registry in the new test case.

(An earlier version of this PR was unfairly blaming dateutil.tz.gettz for incorrect tzinfo; the problem was actually unexpected data cached in icalendar.__tzidMap by other test cases. Maybe should consider a TestIcalendar.setUp() that clears the tzid registry? BTW, testfiles/timezones.ics has pre-2007 data.)

I'm still seeing 28 timezones where there are differences between the VTIMEZONE from pytz and the one from dateutil.tz.gettz (both using tzdb 2016d). I think some of these may be timezones that violate the assumptions in TimezoneComponent.settzinfo(), which would be beyond the scope of this PR.

@untitaker
Copy link
Contributor

👍, this PR fixes the problem I'm encountering at #18

@liZe
Copy link

liZe commented Aug 1, 2016

@skarim Is there anything we can do help you merge this PR?

@esalazar esalazar added this to the 0.9.3 milestone Aug 26, 2016
@esalazar esalazar merged commit f19150b into skarim:master Aug 26, 2016
@wpercy wpercy mentioned this pull request Dec 7, 2016
lucc pushed a commit to lucc/vobject that referenced this pull request Apr 9, 2024
Add basic tests for Radicale issues
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.

4 participants