Add default end date/time for events with no end time or duration

[dannycjones]

Hey! Looking to fix #257.

Before this change, some events would be parsed and result in a ValueError. According to RFC 5545 section 3.6.1, there are default behaviors to be assumed in these cases - namely that events with a starting date should assume the event is one full day, while events with a starting time and date should assume zero duration.

Please let me know if there's any style conventions or test conventions to fix, I'm no Python expert.

[allenporter]

Looks great overall, thank you for the fix and I see where this was missed from the rfc.

Would you also be up for adding an ICS file in tests/testdata and verifying test_calendar_stream.py? You can refresh the snapshots with py.test tests/test_calendar_stream.py --snapshot-update. The unit tests are great, though i also like building up the collection of ics files to test with as part of fixing the bugs, and this will also show that the behavior when serializing the content back out to disk (your change doesn't explicitly set an end field the ics content, which seems like the right behavior)

[allenporter]

Looks great overall, thank you for the fix and I see where this was missed from the rfc.

Would you also be up for adding an ICS file in tests/testdata and verifying test_calendar_stream.py? You can refresh the snapshots with py.test tests/test_calendar_stream.py --snapshot-update. The unit tests are great, though i also like building up the collection of ics files to test with as part of fixing the bugs, and this will also show that the behavior when serializing the content back out to disk (your change doesn't explicitly set an end field the ics content, which seems like the right behavior)

I gave you the wrong instructions here. I thought I had upgraded this library to use syrup snapshots, but i got it mixed up with another library. It's --update-goldens

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (28f39e7) 96.90% compared to head (bd4ac88) 97.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
+ Coverage   96.90%   97.05%   +0.14%     
==========================================
  Files          47       47              
  Lines        2812     2814       +2     
==========================================
+ Hits         2725     2731       +6     
+ Misses         87       83       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[allenporter]

Looks great overall, thank you for the fix and I see where this was missed from the rfc.
Would you also be up for adding an ICS file in tests/testdata and verifying test_calendar_stream.py? You can refresh the snapshots with py.test tests/test_calendar_stream.py --snapshot-update. The unit tests are great, though i also like building up the collection of ics files to test with as part of fixing the bugs, and this will also show that the behavior when serializing the content back out to disk (your change doesn't explicitly set an end field the ics content, which seems like the right behavior)

I gave you the wrong instructions here. I thought I had upgraded this library to use syrup snapshots, but i got it mixed up with another library. It's --update-goldens

Actually -- we already have an example test file that does not have a duration, however the existing tests don't catch this. In I #259 i just added a test that coincidentally failed due to the error that you are fixing here, but i had to exclude it since your fix is not in yet. If you sync to head and remove the guard for rrule-yearly.yaml that I just added we should be good to go without you need to add another example ics file.

[dannycjones]

Thanks for the quick feedback. Just pushed those changes.

It also includes a new ICS file, but happy to remove if you prefer.

[allenporter]

Some of the tests are failing because ical is not preserving the extended attributes (not really supported yet) and some other sort orders change. You can either remove that yaml test (given we have another test covering some cases), remove the attributes, or add to the yaml (like some other tests have) where it has a different ordering for the serialized output.

[dannycjones]

I'll drop them for now since we already have the other test. :)

[allenporter]

Looks great, thanks @dannycjones 👍🏼