CreateEventFromVOBJ: if there is no dtend, use the duration if available #282
Conversation
|
Would you be willing to rebase off current master and then do a bit more testing? I think it would be good to add your calendar file to the tests/data/ directory and set up some sort of unit testing for your code? |
tschwinge
force-pushed
the
dtend-duration
branch
from
58fe681
to
2ec1c9b
Compare
There was a problem hiding this comment.
Thank you for this PR and for your patience with me waiting on this review.
The only thing further I'd like to see, beyond the minor points in the review, is a test. IIRC there is already a test VCARD file in the tests/data dir, so putting an example ICS file in there to test against should be relatively straightforward since you have a model to go by.
| print('Local End....%s' % | ||
| self._localize_datetime(ve.dtend.value) | ||
| ) | ||
| #if not hasattr(ve, 'dtend') and not hasattr(ve, 'duration'): |
There was a problem hiding this comment.
I don't think we want to merge changes that comment out blocks. We should just delete unused code.
| else: | ||
| event['end'] = event['start'] | ||
| #DebugPrint('event['start']: %s\n' % event['start']) |
There was a problem hiding this comment.
Let's remove these comments too.
| print('Calculated End........%s' % end.isoformat()) | ||
| print('Calculated Local End..%s' % self._localize_datetime(end)) | ||
| # Decide based on dtstart; that's what we base our | ||
| # calculation on. TODO: correct? |
There was a problem hiding this comment.
Can you follow up on this TODO?
|
@jcrowgey: Thanks for looking into this, and no worries about the delay. I also haven't really been proactive with pinging this. Thanks for your review; that all makes sense, and I'll re-work/extend as suggested. I won't get it done this year, but I'll try to before the "third birthday" of this PR. ;-) |
Now 7 years old! Do you still have interest in this change or would you prefer to abandon and let someone else pick it up later? |
|
K, I rebased a version of these changes onto the latest main in #771, doing some polishing and then getting that merged. |
... which allows me to
gcalcli importthe*.icsfiles provided by a travel agency. According to a quick skim of one random ;-) iCalendar RFC, that appears to be legit.Beware: only very lightly tested, so far.