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

A few enhancements to icalevents #66

Merged
merged 1 commit into from Mar 12, 2020
Merged

Conversation

@ndw
Copy link
Collaborator

ndw commented Mar 10, 2020

This PR contains the same changes as my earlier PR (#53) reworked so that they apply cleanly to the current master. Along the way, I made a few improvements so that all of the tests pass. In this PR:

  • Fixed mangling of the timezone see #12 (comment)
  • Track all of the timezones defined; attempt to map events to the correct timezone
  • Ignore dates listed as exceptions (i.e handle EXDATE on recurring events)
  • Recompute the start time of recurring events in the current timezone (preventing recurring events from being off by an hour if the recurrence crosses the daylight savings time boundary)
  • Make sure that attendee and organizer are always present

I've tried to document the reasoning behind each of the major changes.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #66 into master will decrease coverage by 0.44%.
The diff coverage is 84.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   83.27%   82.82%   -0.45%     
==========================================
  Files           4        4              
  Lines         269      326      +57     
  Branches       66       77      +11     
==========================================
+ Hits          224      270      +46     
- Misses         25       31       +6     
- Partials       20       25       +5
Impacted Files Coverage Δ
icalevents/icalparser.py 83.12% <84.05%> (-0.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c377ab...11dfa8c. Read the comment docs.

@Hultner

This comment has been minimized.

Copy link
Collaborator

Hultner commented Mar 12, 2020

Very nice, looks like I can decommission my local patch when this is merged :)

Copy link
Collaborator

Hultner left a comment

Did a quick review of the code. Feel free to disregard it if you feel it's irrelevant.

# contain start/end times for daylight
# saving time. Get the system pytz
# value from the name as a fallback.
timezones[name] = timezone(name)

This comment has been minimized.

Copy link
@Hultner

Hultner Mar 12, 2020

Collaborator

Would it be wise to to log a warning here?

This comment has been minimized.

Copy link
@ndw

ndw Mar 12, 2020

Author Collaborator

I don't think it's necessary. If the timezone isn't known locally, the call will fail. If it is known, the only case where it matters is if the rules for daylight saving time have changed.

@@ -44,6 +45,8 @@ def __init__(self):
self.created = None
self.last_modified = None
self.sequence = None
self.attendee = None
self.organizer = None

This comment has been minimized.

Copy link
@Hultner

Hultner Mar 12, 2020

Collaborator

Nice addition

# timezone from the start time, we'll have lost that.
ecopy.end = dtstart + duration

exdate = "%04d%02d%02d" % (ecopy.start.year, ecopy.start.month, ecopy.start.day)

This comment has been minimized.

Copy link
@Hultner

Hultner Mar 12, 2020

Collaborator

Hmm, not really a question for this PR but maybe more for the project as a whole but wouldn't it make sense to use the more and more readably f"…"-strings or "…".format(…)-strings if we want to support python 3.5 and prior versions?

Python 3.5 is EOL later this year https://devguide.python.org/#status-of-python-branches
If supporting it is still a priority we could use .format(), if not I'd go with f-strings.

This comment has been minimized.

Copy link
@ndw

ndw Mar 12, 2020

Author Collaborator

I don't feel strongly about it; but yes, doing it the same way throughout probably makes sense.

#TODO: What about rdates and exrules?


# TODO: What about rdates and exrules?

This comment has been minimized.

Copy link
@Hultner

Hultner Mar 12, 2020

Collaborator

Is this something we should worry about?

This comment has been minimized.

Copy link
@ndw

ndw Mar 12, 2020

Author Collaborator

Dunno. I don't think that's my comment, I must have just put the space in reflexively.

@irgangla irgangla merged commit b29c7f9 into irgangla:master Mar 12, 2020
3 checks passed
3 checks passed
codecov/patch 84.05% of diff hit (target 83.27%)
Details
codecov/project Absolute coverage decreased by -0.44% but relative coverage increased by +0.78% compared to 3c377ab
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@irgangla

This comment has been minimized.

Copy link
Owner

irgangla commented Mar 12, 2020

@Hultner @ndw Do you want to maintain this project?

@ndw

This comment has been minimized.

Copy link
Collaborator Author

ndw commented Mar 13, 2020

I'm happy to help, I'm not sure I have the bandwidth to take over.

@Hultner

This comment has been minimized.

Copy link
Collaborator

Hultner commented Mar 16, 2020

@irgangla I can certainly help maintaining it, if you want a new primary maintainer I have to think about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.