Implicit data conversions and changes to an object - should we change the API?

[tobixen]

Today an object can be edited like this:

my_event = calendar.search(...)[0]
my_event.icalendar_component['summary'] = "This is the edited summary"
my_event.save()

This was my invention, so of course I like it - but there are troubles here. First of all, the side effect. Consider those code-lines:

my_event.data
my_event.icalendar_instance
my_event.vobject_instance
my_event.data

It looks like a noop, but this little sequence of commands will efficiently convert the data from a string to an icalendar_instance then to a vobject_instance (via a string), and finally back to a string again. This may cause some problems:

• While the semantics of the object shouldn't be changed by the conversions, the actual data representation may change.
• Ii've grown up in an age where it was important to optimize things not to use too many CPU-cycles and memory. The conversions above makes me cringe a bit - I would guess quite much CPU is spent on wasteful work here.
• In quite some of the internal methods I was earlier always using my_event.data to avoid conversions - but at some point I gave up, after all we do have an icalendar library for doing icalendar operations for us. Meaning that just internally an object may be tossed from one format to another during ordinary usage of the library.
• The data conversion may also cause a data loss in some corner situations. The code below will NOT edit the summary:

my_event = calendar.search(...)[0]
icalendar_component = my_event.icalendar_component['summary']
my_event.data ## now icalendar_component is no longer connected to my_event!
icalendar_component['summary']  = "This is the edited summary"
my_event.save()

Should the API be changed now that before we're releasing 3.0?

I do remember @niccokunzmann reaching out to me earlier on this topic having some thoughts about it, but I can't find back to the conversation.

[niccokunzmann]

Looking at how I would like to use it:

Suggestion 1: One source of information (switching according to usage)

search = calendar.search(...)
caldav_event = search.events[0]  # can have other attributes. I assume I know what I want
assert len(search.calendars) == 0 # also for the others if I just look for events.
assert len(search.components) == search.events, "I only searched for events"

assert caldav_event.component is caldav_event.event  # this is also very nice for type hints: You know the class of this attribute.
assert caldav_event.todo is None, "I have an event here"

assert caldav_event.data == caldav_event.event.to_ical()
assert caldav_event.is_valid()  # use vobject for this, based on the .data attribute

# changes happen
caldav_event.event.summary = "new summary"

# save the changes
caldav_event.save()

If you refactor the attributes into properties, then you can remove the side effects between them.
I would use a Strategy pattern for that to be perfectionist:

class RawDataStrategy: # has the raw data as input. Can do validation. Switches to other strategy if needed.
    def __init__(self, data, caldav_cmoponent):


    def get_data():
        return self.data

    def get_component():
        self.caldav_component._switch_to_parsed_component(ParsedComponentStrategy(icalendar.Event.from_ical(self.get_data()))

    def validate():
         # pass get_data() to vobject

    def set_data(): # if you like that

    def save():
        # use the data to to post to the server

class ParsedComponentStrategy:

    # save the event in __init__
    # same methods as all other strategies
    

    def save(): # use self.component to post to the server

You can have a new strategy for only vobject usage.

suggestion 2: user handles side effects.

You only provide get_data, set_data and parse_component and save_component without the internal state of the component.

[tobixen]

I should think more about this when I have better time, but before I forget it I will write down some arbitrary thoughts:

• I should definitively offer .get_foo and .set_foo (where foo can be vobject, icalendar_instance, icalendar_component or data (string). as an alternative to just using caldav_event.foo. There could be a parameter in the get_foo indicating weather the caller wants a disassociated copy or an object intended for editing.
• The "data"-representation is not a big problem, particularly if using the "strategy pattern" as mentioned above. If the caldav_event is already converted to an icalendarobject, then caldav_event.data=bar could simply modify the icalendar object. Strings are immutable, we don't have to worry about people modifying the string representation.
• The "strategy pattern" looks good, I could put that to good use.
• Now, icalendar and string representation is two things ... but I'm also supporting conversion to vobject, and probably there will be other formats later (I should look into how jcal is supported in the icalendar library, for one thing). What I could do is simply saying that first converting to an icalendar object and then to a vobject is a very rare corner case that we don't need to think about, and raise an error or throw a warning should it happen. One problem with that is that the icalendar object is used internally for things like finding the UID of an object. Again, this may be solved i.e. by using the strategy pattern.

[niccokunzmann]

Cool. Let me know if you like to have more input on this.

[tobixen]

Here are some AI-generated thoughts on this: https://github.com/python-caldav/caldav/blob/v3.0-dev/docs/design/DATA_REPRESENTATION_DESIGN.md

[niccokunzmann]

For that, there should be a Null Object Pattern:

        if data:
            self._strategy = RawDataStrategy(data)
        else:
            self._strategy = NoDataStrategy()

return self._strategy.get_vobject_copy() if self._strategy else None

becomes:

return self._strategy.get_vobject_copy()

---

This misses the point I was making, I think: https://github.com/python-caldav/caldav/blob/v3.0-dev/docs/design/DATA_REPRESENTATION_DESIGN.md#state-transitions

Either, you need to have a single source (icalendar if parsed, else data string) if caldav holds the state OR you pass the state responsibility on the the user. This suggestion is confusing for me because it does not solve the underlying issue that the responsibility is not clear.

Another idea I just had is based on RUFF's borrowing: https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html
That means: IF you want to modify something, you need to borrow it an nobody else CAN edit it at the same time. Then, you give it back.

with caldav_event.icalendar as calendar:
    # do the edits and close

with caldav_event.vobject as calendar:
    # do the edits and close

The old side effects:

my_event.data  # stay in read-only mode data strategy
my_event.icalendar_instance.open()  # switch to icalendar strategy AND borrow the instance
my_event.vobject_instance.open()  # fails - because you are still using the other representation
my_event.data  # fails because the first one is not closed

The new way of writing it:

with my_event.data as data
    # noop - data is immutable but now you know that nobody changes it at the same time

with my_event.icalendar_instance as calendar:
    calendar.events[0].summary = "new summary"

with my_event.vobject_instance:
    # ... do verification, transition

The main overhead and delay you will be facing is probably the network, not the copying of objects. Python is quite fast with this, I think.

The state diagram looks as follows: (Maybe we have a state pattern)

This way, it is clear who holds the responsibility. You can also use that functionality internally.
E.g. this:

with my_event.icalendar_instance:

    # some function calls deeper
    with my_event.icalendar_instance:  # can work because you are already editing. You should probably count then :)
    

---

One problem with that is that the icalendar object is used internally for things like finding the UID of an object. Again, this may be solved i.e. by using the strategy pattern.

Yes. E.g. if the VObjectStrategy does not implement some things, one can convert to using icalendar and then return that. I think, "Strategy" should be replaced by State or something like that because now, it becomes a state machine.

[tobixen]

I don't have time looking and thinking deeply and thoroughly through this now, but I really like the idea of using the with-statement.

[tobixen]

Suggestion 1: One source of information (switching according to usage)

search = calendar.search(...)
caldav_event = search.events[0] # can have other attributes. I assume I know what I want
assert len(search.calendars) == 0 # also for the others if I just look for events.
assert len(search.components) == search.events, "I only searched for events"

Changing the behaviour of calendar.search(...) is outside the scope of this issue :-) python-caldav/icalendar-searcher#1 would be a more appropriate place to discuss it, maybe :-)

I do see the benefits wrg of type hints.

I would rather avoid .calendar, in the caldav context it's important not to confuse caldav calendar collections with icalendar objects.

assert caldav_event.component is caldav_event.event # this is also very nice for type hints: You know the class of this attribute.

vobject has this (obj.event, obj.todo). I do see the point with the type hints, but I'm concerned it may be abused. There is an "anti-pattern" I've seen many times (the caldav library used to depend on vobject before), both in examples, usage and pull requests (I've even been sleepy enough to approve one such pull request). People write code that could be equally applicable to events as tasks (in the icalendar protocol those are very similar after all), but instead of using obj.component they use obj.event. The latter makes the code more readable (everyone knows what an "event" is, but what is a "component"? I've even made it into a documentation standard to simplify things by using the word "event" consistently when I mean "event, todo or journal"). In the end one ends up with code that unexpectedly throws away all components that aren't events!

assert caldav_event.todo is None, "I have an event here"

The other anti-pattern I've seen many times by users of the vobject-library is to duplicate code - do things once for events, then repeat the same code for todos, and then forget that there may exist such a thing as journals too!

[tobixen]

So now I'm considering some kind of hybrid solution:

Backward compatibility

We should still have full backward compability at least for some years to come, and ideally backward compatibility with deprecation warnings for some more years. So caldav_event_object.icalendar_component['summary'] = 'new summary' should still work, although no more the recommended way of doing things, and may stop working in some future version of the library. Perhaps it should be explicitly stated in the documentation that the behaviour is undefined when doing things like this. Alternatively, it would be better to decorate/modify/wrap the icalendar objects returned so that in v4.0 a legacy warning is issued when trying to modify it - and later, in v5.0, an error would be thrown. (Claude recommends to simply document the behaviour as undefined).

"Mutable borrowing" - editing in a context block

I must say that I really like the idea of using the with-statement as a way to flag that "in this code block I will use this representation of the object and do changes on it". Even with no logic at all in the context manager, it already helps as it forces the library users to think about when and why the object should be muted, and forces the library user to be "quick" with it.

I should probably set up some locking solution making sure that exclusive rights to edit the object is given when entering the with-block. However, I don't think it's essential to spent time on it now - I think the important thing when launching v3.0 is to just make sure the with-pattern works, and that it's documented as the best current practice.

Explicit getters and edit

If calling my_caldav_event_object.get_icalendar_instance(), then a copy (or, possibly, a read-only object) would be returned.

Claude recommends using .edit_icalendar_instance() for getting the context manager. Overloading the caldav_event.icalendar_component property to both work as a context manager and work as an icalendar component may be tricky, and having a dedicated method for this makes the code more explicit.

Strategy pattern

One of the thoughts with the strategy pattern was to allow this to work:

foo = my_caldav_event_object.component
my_caldav_event.data = "(...)\nSUMMARY:New summary\n(...)\n"
assert foo['SUMMARY'] == 'New summary'

But I'm considering to skip this.

However, we do access the properties internally in the library. We should probably use the strategy pattern to avoid having to convert the data forth and back too frequently. I had the AI make an overview of when and why. I will look through this and think a bit.