Remove arrow, transition to attrs and add Timespan

[N-Coder]

This implementation collects all time-related attributes of an event - begin, end/duration and precision - in a single, isolated and easy to test class. It also targets the three possible cases for time values - timezone-aware datetimes, floating datetimes (which are currently unsupported, for details see further down) and all-day dates (which are currently pretty buggy). Additionally to not doing anything implicitly (neither any fancy value conversion nor fancy indirect property access), it viciously guards the validity of its internal state. The validity of each of the 4 values representing the event time is highly depending upon the other values. Allowing mutation opens up easy ways of breaking validity - so the implementation is actually immutable, with each modification creating a new object and requiring a full revalidation of all 4 values together. As the times shouldn't be modified that often, having the optimization of mutability shouldn't be strictly necessary. Additionally, moving the functionality to its own class makes testing easier and keeps this quite complicated functionality clean from other concerns. All this makes the timespan handling easier to implement correctly, but unfortunately not easier to use. So I also started modifying the Event class to handle the interfacing in an user-friendly way and hide the complexity of timespans.

This is by far not completely integrated in the rest of the library, as I first wanted to see where this approach might lead us and now also wanted to collect feedback. See below on how we might continue from hereon.

Where did Arrow go?

I first started out with the realisation that floor('day') doesn't suffice for date values, as we need to get rid of timezone:

morning_1 = arrow.Arrow(2020, 1, 19, 8, 0, tzinfo="+01:00")
morning_3 = arrow.Arrow(2020, 1, 19, 8, 0, tzinfo="+03:00")
print(morning_1, morning_1.floor("day"))
# > 2020-01-19T08:00:00+01:00 2020-01-19T00:00:00+01:00
print(morning_3, morning_3.floor("day"))
# > 2020-01-19T08:00:00+03:00 2020-01-19T00:00:00+03:00
print(morning_1.floor("day") == morning_3.floor("day"))
# > False

Unsetting the timezone does strange things, as the constructor always interpolates tzinfo=None to UTC:

midnight_None = arrow.Arrow(2020, 1, 19)
midnight_None.tzinfo = None
print(midnight_None, midnight_None.clone(), midnight_None.floor("day"))
# > 2020-01-19T00:00:00 2020-01-19T00:00:00+00:00 2020-01-19T00:00:00+00:00
print(midnight_None == midnight_None.clone())
# > False
print(midnight_None == midnight_None.floor("day"))
# > False

Trying to use arrow for naive (i.e. floating) timestamps by keeping tzinfo set to None is thus very hard. So we would need another variable to keep track of whether the tzinfo is intentionally set or should actually be None. This would require a lot of special casing, e.g. when comparing a timestamp with timezone +3 with one with UTC, as we need to check whether UTC actually means floating.

To summarize, I'm not quite happy with the automatic timezone mangling that arrow does. It might be useful to do things better when you don't worry about timezones, but here we have to actually get timezones right ourselves without something automagically mangling them. Python's built-in datetime might be annoying, because it is a lot harder to use right and tends to throw exceptions (e.g. when comparing timezone-aware datetimes with timezone-naive/floating datetimes). But in those special cases guessing right automatically is hard. Additionally, these exception point us at places where we might need to handle things explicitly - e.g. when mixing timezone-aware datetimes, floating datetimes and all-day dates that don't have a timezone.

As a result, for me, the selling points of arrow no longer outweigh its deficits. Too many modules and types is not an argument here, as this should be a library and not some prototype code that you are typing into the interpreter shell. Doing conversions automatically and not properly handling naive timestamps is the root cause for the above problems. And all the other features could also be implemented on top of plain old datetimes - actually the Arrow class is only a thin wrapper around the datetime class, adding some functions but no further information (!). As a result, my timespan implementation only uses the plain, old, but reliable datetime class.

Further steps

So, I tried to do a timespan implementation based on datetime and tried to cover all edge-cases I could come up with. For those that I didn't think of, it should trow an exception so that we can think of the proper way of handling it once it comes up, instead of it vanishing through some magic conversion resulting in probably wrong results.

This should somewhat follow the zen of python (see import this)

Explicit is better than implicit.
[...]
Errors should never pass silently.
[...]
In the face of ambiguity, refuse the temptation to guess.

And now finally make_all_day looks good - see the quite extensive test for this function. Additionally, this functionality would now allow floating times properly, as they are not that different from dates. Actually, it should fix all the issues referenced in #155 (comment). If we want to go this way, we would probably need to make changes in a few other places and think about breakages in the public API. But, this would also allow us to finally properly conserve floating events and VTIMEZONES when parsing and later exporting an .ics file. What also still needs testing is handling of events with different begin and end time zone - e.g. plane flights. But I'm confident that those will also work reliably, given that we are able to define the right expectations and guarantees we have/want in these cases.

@C4ptainCrunch what's your opinion on this? Should we try to continue on this way, potentially removing arrow support in many places for the sake of correctness? I'd love to finally see this working properly and I could try to get a complete version with this new functionality working, if you also think that this is the way to go.

[C4ptainCrunch]

@N-Coder This really looks like you put a lot of thinking into that and seems really smart ! :)
I'm also all in to remove Arrow, i wanted to remove it since a long time but never had the courage.
Also, no problem in breaking the public API, that's why we are still in beta.

[C4ptainCrunch]

I also see that i don't have much time to handle ics.py at the moment and you seem very motivated to work on it. Would you like to have write rights on the repo ? I'd be glad to give them to you!

[N-Coder]

@N-Coder This really looks like you put a lot of thinking into that and seems really smart ! :)

Thanks!

I'm also all in to remove Arrow, i wanted to remove it since a long time but never had the courage.
Also, no problem in breaking the public API, that's why we are still in beta.

Very good, then I'll try to completely get rid of Arrow.

I also see that i don't have much time to handle ics.py at the moment and you seem very motivated to work on it. Would you like to have write rights on the repo ? I'd be glad to give them to you!

Motivated yes, but I unfortunately don't have that much spare time either. 😅
Having multiple persons with write access to reduce the truck-factor seems to be a good idea nonetheless.
I'd anyways first collect my changes in this PR and wait some time for feedback before merging them. But maybe we should talk about when/how to release a new version already now, as that might be appropriate once the changes are in.

[N-Coder]

@C4ptainCrunch do you have an Opinion on attrs / python 3.7 dataclasses? They would make declaring all our data container classes a lot easier and less repetitive, esp. they would autogenerate most of the constructors and prevent bugs like #217.

[N-Coder]

Arrow is gone from the library code with 02a5904!
Now that floating timestamps are also allowed throughout the library, we should add tests for them. As they need special attention when comparing with aware timestamps, the normalization done for the comparison operations of Timespans/Events and also the Timeline that highly relies on them need a lot of testing.
The commit also fixes #188, but we still need to take some extra care of the timezone during parsing / serialization (i.e. implement #129).

So the things still left to do are:

• also remove arrow from the tests and make them pass again
• add tests for Timeline operations and Timespan comparability (esp. with diverging timezones)
• ensure that correct parse_date(time) is used when parsing
• keep track of used_timezone information from serialize_datetime_to_contentline and generate VTIMEZONEs

Here's the updated table of affected issues from #155 (comment):

PR	Issue	Description	Author
#174	#173	don't convert timezone of dates	N-Coder
#159	#155	Improve all-day event handling	perette
#94	#92 / #150	Event.end property adds extra day to Event.end	marianobrc / wom-bat
#144 / #152		Fix all-day events lasting multiple days	raspbeguy
	#161	batch timezone conversion of all events	perette
	#129	programmatical / serialization VTIMEZONE support
	#181	timezone information of time fields is ignored	tgamauf
	#182	timestamp seconds are ignored	tgamauf
	#188	timezone information is dropped	moccacup
	#216	update Arrow	christofsteel
#168 / #180	#14	recurring events (RRULE)	timfaner / lengthmin / btrevizan

[C4ptainCrunch]

Hey 👋

Motivated yes, but I unfortunately don't have that much spare time either. sweat_smile
Having multiple persons with write access to reduce the truck-factor seems to be a good idea nonetheless.

I just gave you the rights to push on master 🎉

Arrow is gone from the library code with 02a5904!

This is awesome, thanks ! 👍

---

About attrs, i'm not sure (but not definitely opposed either) because of 2 reasons:

• First is the added dependency, i'd like to keep them as few as possible
• Second is that i'm not sure it's a good idea to have every and all attribute settable in the constructor. I know it's the case at the moment, but if we look at the current signature, it's not really pretty nor useful (i think). But changing this would be quite a big change and would require some thinking (at least to choose what attributes are important and should stay in the constructor)

What do you think ?

[N-Coder]

Thanks! 👍

Regarding attr: I wouldn't see a problem with the added dependency, as the library is pretty small and used pretty much everywhere anyways. Some of its functionality was included in the standard library in 3.7, so you can kind of consider the library as a future-proof backport adding some more features. You can check out their docs for more details. They also have some very convincing arguments why you actually don't want to do it differently (Glyph also wrote a very graphic article about that). Regarding your second point, controlling constructor arguments is pretty easy using the per-attribute init argument (which also works with the stdlib version). One of the features attr adds over the stdlib version is automatic validation and value conversion (both for the constructor and any setter), which would make it very easy to ensure that we're only carrying valid data around, no matter how we obtained it (preventing bugs like #207 and #208).

Note: when doing the migration, watch out for CREATED vs DTSTAMP #200.

[N-Coder]

Maybe we should do one small release before incorporating all these changes, including fixes for the following low-hanging fruit (see the version 0.7 milestone):

• Bugfix in Event constructor self.organizer is None #214 / organizer param on Event.__init__() doesn't do anything #217
• Doesn't work with Python < 3.8 since latest TatSu release #221 / use last compatible tatsu 4.x before python 3.8 #229 (waiting for example / can't reproduce)
• Timeline.today() requires useless day argument #224 / remove useless day argument from Timeline.today() #228
• ParseError if emojis/unicode in description/text #206 / invert the regex matching contentline values to also allow emojis #227 (emojis fixed, but still no line folding as in Long DESCRIPTION has embedded newline #210/ics does not support 5545 3.1 (long line folding) #215)
• Organizer output is wrong #207 + After parsing an iCal file, event.attendees is empty #209 + No way to add partstat or role to Attendee #218 / Fix Attendee and Organizer Parsing and Serialization #232
• TypeError: Can't instantiate abstract class EmailAlarm with abstract methods action #213 / Add missing implementation for abstract EmailAlarm.action property #233
• The VERSION property should be the first property on the calendar.  #55 / Make sure version is given first #234

Would that be okay for 0.7? The only problem that might arise is the new signature make_all_day(become_all_day:bool) added by ed14cde and the new day-end vs 24h duration rounding logic.
My version does the "unsetting" differently and always uses the day-end logic as it gives reliable results even with weird values. If users want any other logic, they can just round the begin/end/duration values themselves using their preferred scheme and call make_all_day, which shouldn't make any changes if they rounded correctly.
So I'm suggesting to revert perette's changes and only have small changes for 0.7 and then make 0.8 the big release with all the major timestamp/-zone and all-day related changes. This is done in #231.

Could you help me out with the release process once the above is ready?

[C4ptainCrunch]

So I'm suggesting to revert perette's changes and only have small changes for 0.7 and then make 0.8 the big release with all the major timestamp/-zone and all-day related changes.

Seems good to me !

Could you help me out with the release process once the above is ready?

Of course ! Ping me when you need help :)

[N-Coder]

Yep, I wouldn't want to code without it anymore. I was actually also thinking whether attrs could help us with parsing and serialization by adding metadata to the attributes. As many of the parsing / serialization methods are simple and quite repetitive, we could then generate those automatically. But I'll wait with exploring that until this PR is through.

[tomschr]

Commit 62ea485 now nicely defines how Timespans, Events and Todos can be compared. We should probably document the consequences and add some tests to visualize (and also prevent regressions of) the exact behaviour. Maybe we can even combine both and have some more examples in the documentation, which are tested automatically against the current code base. @tomschr do you have an opinion on this?

@N-Coder If I understood you correctly, sure, that would be a good idea. Of course, examples would help a lot. The question is, where to include it into the documentation. As the PR hasn't been merged to master yet, it might get a little difficult to add this. 😉

[N-Coder]

The tool I was thinking about (but couldn't recall its name) was doctest. It simply takes any docstring in a python file or any other documentation text file, extracts all blocks of python interpreter sessions it finds, tries to run the exact same input against the latest version of the code again, and checks whether the actual output matches what is written in the documentation.
I made an example and put it into a gist here (scroll down a little to see both files).
This not only makes a good introduction to basic usage of event times, but also replaces some of the very basic tests of the test suite. Actually, it pointed me at a bug in the current version of this PR, where the usage pattern I was expecting diverged from what was actually implemented.
Of course, this can't replace all unit tests, especially for more complicated and corner cases and when you want to run a test against a list of different parameters (for the example of the comparison functions, you probably want to show their basic functionality in the documentation, but have a separate unit-test that ensures sorting stability for a long list of events with different parameters, no matter their order).
But, this kind of combines the tedious tasks of also writing simple regression tests and writing a tutorial for new users, yielding a lot of synergies.

[tomschr]

@N-Coder

The tool I was thinking about (but couldn't recall its name) was doctest. [...]

I see what you mean. That's a good idea! Based on your idea, may I extend it? 😉

As we use pytest for testing, we could use this framework to do (doc)testing as well. You can still write your doctests. The best part is: you can do it in separate files, in your documentation, or both.

What you need:

1. Enhance setup.cfg with the following lines:

   [tool:pytest]
   testpaths = ics tests docs
   addopts =
       # ...
       --doctest-glob='*.rst'
       --doctest-modules
       --doctest-report ndiff

   Here, you've added docs directory

2. Add a tests/conftest.py file with the following content:

    import pytest
    import ics
   
    @pytest.fixture(autouse=True)
    def add_ics(doctest_namespace):
        doctest_namespace["ics"] = ics

   This is needed, as it allows to access all the functions in the doctests without an import. That import is done by the conftest.py file.

3. Add your tests in your RST files. If you don't adapt the --doctest-glob option, it will investigate all RST files. That may or may not what you want. It's possible to restrict the doctest to something test_*.rst, for example.

4. Run your test suite with option -v to see all the details.

---

There is only one, minor issue: inside the documentation, you can only use a small part of the whole test (as an excerpt to show some things). But I don't think this is a big problem. You can still add test_*.rst files which covers the rest.

What do you think? Would that be an idea to pursue?

[N-Coder]

That sounds very good! If was also thinking about using the unittest integration and your proposition sounds like just using the pytest specific variation of that (I'm not too experienced with python testing frameworks and their differences, so thanks for probably saving me some googling on how to make it work in our case 😅).

Regarding the "minor issue", we anyways need to think about how to draw the line between doctests for introductory tutorials or advanced-user documentation (or in other locations with different purposes), expect-like test_*.rst doctest scripts and full-featured pyunit unit-tests. But I'm very sure that being able to do / having all 4 will cater for every need.

In addition to my previous basic Event example, I also did one for the more advanced comparison logic and included both in this commit in this PR. Both are still in Markdown format, so they still need conversion to rst (which should be easy if you know what to look for) in addition to being moved to the right location.

I guess my changes here are close to reaching a point where they are mature enough to be put into a v0.8-dev branch. Handling datetime timezones and ics vTimezones, recurring events, finally bullet-proof all-day events and also further things like correct line-folding and new parsing/serialization to different formats can then be implemented in separate PRs based on that branch.

As the PR hasn't been merged to master yet, it might get a little difficult to add this.

Would that new branch help? Could your rebase your doc PR onto that v0.8-dev branch (there shouldn't be any conflicting changes to the docs outside of the code) and than try to enabled doctest?

[tomschr]

That sounds very good! If was also thinking about using the unittest integration and your proposition sounds like just using the pytest specific variation of that (I'm not too experienced with python testing frameworks and their differences, so thanks for probably saving me some googling on how to make it work in our case 😅).

Hah, great! 👍 For more details, refer to https://docs.pytest.org/en/latest/doctest.html.

Regarding the "minor issue", we anyways need to think about how to draw the line between doctests for introductory tutorials or advanced-user documentation (or in other locations with different purposes), expect-like test_*.rst doctest scripts and full-featured pyunit unit-tests. But I'm very sure that being able to do / having all 4 will cater for every need.

Right. It may be a matter of taste where to put it. I see the doctests in documentation more of having some good examples to help our users. As a nice side effect, we can test it. 😄

I did that with another project and it showed their benefits instantly: it helped me to discover typos and outdated information. That makes maintaining documentation a least a bit easier as it gives you feedback once there is something wrong.

Both are still in Markdown format, so they still need conversion to rst (which should be easy if you know what to look for) in addition to being moved to the right location.

You can do the conversion with pandoc automatically.

Could your rebase your doc PR onto that v0.8-dev branch?

Sure, I can try.

[N-Coder]

Okay, I guess the removal of arrow and the transition to attrs and Timespan is pretty much done.
Mypy and doctest are also happy, and we're left we currently only 4 failing tests:

• test_selfload fails because timezones are serialized and parsed differently
• test_version fails because we seemingly no longer drop further version information appended with a ";"
• test_all_day_repr and test_has_explicit_end fail because I didn't rework the make_all_day tests yet

So there are also still quite a lot open points on the todo list (which should at some point maybe move to one or more meta-issues, together with the further topics mentioned towards the end of this comment), but I think this version should be stable enough that further work can be based on it. My next topic would be reworking the serializer and parser logic and getting the handling of timezones right while doing so, but I'd prefer to do that in a new PR. @C4ptainCrunch what do you think, can we merge this PR? Maybe, as suggested above, into a v0.8-dev branch instead of master, as the changes are neither self-contained nor complete.
If you want to squash the commit, I'd suggest something like "remove arrow, transition to attrs and add Timespan (PR #222)" as title

[N-Coder]

Most important breaking changes:

• arrow was removed, use built-in datetime and timedelta instead
• events, todos, attendees and alarms are now all lists instead of sets, as their contained types are not actually hashable and in order to keep the order they had in the file. Use append instead of add to insert new entries.
• attendees and organizer now must be instances of the respective classes, plain strings with the e-mail are no longer allowed
• extra can now only contain nested Containers and ContentLines, no plain strings
• some attributes now have further validators that restrict which values they can be set to, which might further change once we have configurable levels of strictness
• dtstamp and created have been separated, dtstamp is the only one set automatically (hopefully more conforming with the RFC)
• Event.join is hard to do right and now gone if nobody needs it (and is able to formulate a clear behaviour faced with floating events vs events in different timezones and also all-day events)
• method has_end() -> property has_explicit_end as any Event with a begin time has an end

[tomschr]

@N-Coder Hi Nico, not sure if this is helpful, but I had this lying around. 😉

I found some minor typo fixes here and there. Nothing serious. Also I'm not quite sure about the project's policy regarding docstrings for classes and functions. I haven't added a comment for all.

I hadn't had the time to look at the RST files. If you like, I can have a look at them too. 😉

Other than that, this PR is quite impressive! 👍

[tomschr]

Class docstring missing.

[tomschr]

Class docstring missing.

[N-Coder]

Thanks for your review! Honestly, I didn't really touch inline code docstrings. Especially the method docstrings are probably now pretty far from what actually happens in some places. I'm wondering whether it might be a good idea here to first start creating an overview over the big-picture functionality and only afterwards go into documenting the details on each method. There's probably also a lot of common terms / ideas that should be described first and then allow to keep the in-code documentation short.

Regarding class documentation, let's start with the current class hierarchy as it is implemented:

RuntimeAttrValidation(object) (ics.types)*
    Component(RuntimeAttrValidation) (ics.component)
        CalendarEntryAttrs(Component) (ics.event)*
            EventAttrs(CalendarEntryAttrs) (ics.event)*
                Event(EventAttrs) (ics.event)
            TodoAttrs(CalendarEntryAttrs) (ics.todo)*
                Todo(TodoAttrs) (ics.todo)
        CalendarAttrs(Component) (ics.icalendar)*
            Calendar(CalendarAttrs) (ics.icalendar)
        BaseAlarm(Component, metaclass=ABCMeta) (ics.alarm.base)*
            FakeAlarm(BaseAlarm) (tests.alarm)
            DisplayAlarm(BaseAlarm) (ics.alarm.display)
            EmailAlarm(BaseAlarm) (ics.alarm.email)
            NoneAlarm(BaseAlarm) (ics.alarm.none)
            CustomAlarm(BaseAlarm) (ics.alarm.custom)
            AudioAlarm(BaseAlarm) (ics.alarm.audio)
    ContentLine(RuntimeAttrValidation) (ics.grammar.parse)

Entries marked with a star are mostly for internal usage and should be relevant for the public interface of the library. RuntimeAttrValidation is a mixin that automatically calls the converters and validators of attributes when reassigning their values after instantiation. The separation between a SomethingAttrs and the corresponding Something class is only due to these classes requiring more complex init arguments, which can't directly be represented by the attrs generated init. We thus need a subclass to overwrite the attrs generated init, do our magic with the arguments, and then init the attributes as usual. CalendarEntryAttrs is there to collect common elements of both Events and Todos and prevent duplicate code, but this common ancestor has probably no public relevance.
The BaseAlarm class is abstract and custom implementations should use CustomAlarm, but its probably still somewhat useful to have a type for all possible alarms.

So, for public usage, the class hierarchy should look like this:

Component(object) (ics.component)
    Event(Component) (ics.event)
    Todo(Component) (ics.todo)
    Calendar(Component) (ics.icalendar)
    BaseAlarm(Component, metaclass=ABCMeta) (ics.alarm.base)
        FakeAlarm(BaseAlarm) (tests.alarm)
        DisplayAlarm(BaseAlarm) (ics.alarm.display)
        EmailAlarm(BaseAlarm) (ics.alarm.email)
        NoneAlarm(BaseAlarm) (ics.alarm.none)
        CustomAlarm(BaseAlarm) (ics.alarm.custom)
        AudioAlarm(BaseAlarm) (ics.alarm.audio)
ContentLine(object) (ics.grammar.parse)

I'm not quite sure how to properly convey this. Should we add pyi files that only implement this hierarchy and then generate the docs from those? Probably documenting all the common stuff in CalendarEntryAttrs once and then having that propagated to Event and Todo would still be useful, though. Additionally, advanced users might still want to know about the technical details. Can we somehow influence which (parent-)classes are shown in the (basic) API documentation?

I'm not sure how to tackle this, in terms of how to make the resulting documentation the most useful, how to realize this technically (e.g. also how to document attributes and possibly the generated init method and how to make this information transparently visible from subclasses) and probably also at which place to start (i.e. big picture documentation, class documentation, and detailed method/attribute docs).

[tomschr]

Thanks Nico for your detailed message. 👍

[...]
I'm not quite sure how to properly convey this. Should we add pyi files that only implement this hierarchy and then generate the docs from those? Probably documenting all the common stuff in CalendarEntryAttrs once and then having that propagated to Event and Todo would still be useful, though. Additionally, advanced users might still want to know about the technical details. Can we somehow influence which (parent-)classes are shown in the (basic) API documentation?

as all your questions are kind of related, I try to answer them from a different perspective:

1. Public vs. "Private"
   Your first class hierarchy contains classes with a star that are "mostly for internal usage". According to PEP8, shouldn't such classes start with an underscore to indicate this internal usage? I haven't tried it yet, but I think, Sphinx could omit such "private" classes etc. They also won't show up in the (API) documentation.

2. Class hierarchy
   Your second class hierarchy contains all the public classes. I would add this as a short(!) section. Use the file docstring in ics.component package. In other, sub classes, we could create a references to this section.
   I'm not sure, if a pyi file would help here. These files are mostly used for type hints and IDEs. I haven't heard of any other use case in regards to documentation (apart from documenting type hints). My guess is, Sphinx doesn't support pyi files anyway.
   You can document class hierarchies with sphinx.ext.inheritance_diagram.

3. Docstrings
   Sorry if I'm so pushy about docstrings, but there is a reason for that. 😉 Well, docstrings serves some very important purposes: first, they can be extracted to form our API documentation. And second (almost more important), it serves as a guide when you are working in an interactive Python shell.
   I use an interactive Python shell a lot. It's easy to start and you can play around. When I'm unsure about what a function expects or returns, I display the help of this function. It shows the docstring of the respective function. This is mostly more convenient and easier than call your browser, look for the documentation, find the respective class/function/etc. With such docstrings, you have this information at your finger tips.

These are just some ideas. Hope they make sense. 😉

Maybe it's easier to tackle doc relevant issues in another pull request to keep this PR easy and well-arranged.

[C4ptainCrunch]

Thank you for your awesome work @N-Coder and for your review @tomschr

What do you think, can we merge this PR? Maybe, as suggested above, into a v0.8-dev branch instead of master, as the changes are neither self-contained nor complete.

Let's go, i'm merging it ! I'll merge it in master though as it's destined to be the next version anyway :)