97 wip #100
97 wip #100
Conversation
…e reviews for commit d25b425
tobixen
force-pushed
the
97-wip
branch
2 times, most recently
from
51d4b03
to
f192acb
Compare
This reverts commit d9de7c2.
| @@ -130,13 +130,14 @@ def __init__(self, source, start, stop, keep_recurrence_attributes=False): | |||
| self.source = source | |||
| self.start = start | |||
| self.stop = stop | |||
| self.end_prop = end_prop | |||
There was a problem hiding this comment.
This is in conflict with clarity and the class attribute.
| @@ -10,6 +10,8 @@ def test_event_has_summary(calendars): | |||
| def test_recurrent_events_change_start_and_end(calendars, attribute): | |||
| events = calendars.three_events_one_edited.all() | |||
| print(events) | |||
| if events[0][attribute].__hash__ is None: | |||
There was a problem hiding this comment.
no. The test becomes worthless.
There was a problem hiding this comment.
This is an independent issue that should be fixed properly in a separate pull request. I chipped in this code just to get the tests to pass. I did try to hash event[attribute].dt, but then the asserts failed ... hm.
There was a problem hiding this comment.
I sent a few comments. I might chip in some code later but am quite busy, too.
It's good to coordinate that we don't work at the same things at the same time :-) I will apparently be busy for the rest of the day.
|
@niccokunzmann - I did not quite get the problem with having a Except for that, obviously the disabled test (ref collective/icalendar#487) should be reenabled again before the pull request is to be considered to be ready. |
|
There is quite some code here to handle the case where DTSTART is not set for journals and tasks (as mentioned, there may be corner-cases out there related to scheduling where DTSTART is not set even for events). |
|
I think that anything that makes it usable fast without breaking existing tests can be merged. The special cases can come later.
Maybe the abstraction is to have a reference date for the repetitions. Events, journals, ... they have their strategy to deal with reading the component and generating a new one, sitting in between. Hm. I did not have a look at the code, yet. I really should. I find it interesting and this is a major restructuring.
|
|
I wouldn't call it a "major restructuring" - I just split one class into one superclass and three child classes, keeping most of the original logic in the super class :-) |
|
For the test that is failing because of icalendar's hashing: You can make a list comprehension out of it: `set(` -> `[` and `)` -> `]`. I had a look at it and it is the same meaning.
|
recurring_ical_events.py
Outdated
| """Create an event which may have repetitions in it. | ||
|
|
||
| - event - the icalendar Event | ||
| - object - the icalendar object |
There was a problem hiding this comment.
These are called components, I think.
| @@ -130,13 +130,14 @@ def __init__(self, source, start, stop, keep_recurrence_attributes=False): | |||
| self.source = source | |||
| self.start = start | |||
| self.stop = stop | |||
| self.end_prop = end_prop | |||
| self.keep_recurrence_attributes = keep_recurrence_attributes | |||
|
|
|||
| def as_vevent(self): | |||
There was a problem hiding this comment.
My guess is that this will be part of the subclass, too..
There was a problem hiding this comment.
There is one single place relatively deep in a method in the super class that uses this one. I didn't see any obvious way to move that logic to the subclasses without adding code duplication. You're welcome to have a look though :-)
|
Hm. Architecture wise, I'd think composition through strategy pattern instead of subclassing might me clearer. It divides the repetition calculation from the extraction of information from components and the composition of the repeated components. |
|
These are called components, I think.
I've had a brief look at the RFC now and concluded you are correct.
…--
Tobias Brox
Senior System Consultant
Redpill Linpro AS - Changing the game
Mobil: +47 917 000 50
Kontor: +47 215 441 68
|
Sounds like a lot of refactoring - I don't have the capacity for that :-) |
|
I do not have the capacity either at the moment and you don't have to do that. Let's get it working and then make it better. I would think though that it would speed up long term development and fixing and improving the edge cases.
|
|
Uh. The tests are running! I will have a look now ... |
|
Okay. I will merge this now. I is a lot more usable for your use-case than the code before and it does not break any existing code. |
Apparently this doesn't work
#97