Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] as_vevent: use vDDDTypes instead of explicit date/datetime type #19

Merged
merged 1 commit into from Jan 15, 2020

Conversation

wichmannpas
Copy link
Contributor

This fixes a problem when using the decoded method.
E.g., event.decoded('dtstart') fails with an exception:

  File ".../.pyenv/lib/python3.7/site-packages/icalendar/cal.py", line 237, in decoded
    return self._decode(name, value)
  File ".../.pyenv/lib/python3.7/site-packages/icalendar/cal.py", line 220, in _decode
    decoded = types_factory.from_ical(name, value)
  File ".../.pyenv/lib/python3.7/site-packages/icalendar/prop.py", line 1042, in from_ical
    decoded = type_class.from_ical(value)
  File ".../.pyenv/lib/python3.7/site-packages/icalendar/prop.py", line 334, in from_ical
    u = ical.upper()

This is due to a type inconsistency between the event created by this
package and the type that icalendar expects.

Another (ugly) workaround for this problem (if this patch was not
applied) would be to convert the event to ical and parse it back using
icalendar, which repairs the dtstart type inconsistency as well (e.g.,
something like `event = icalendar.Calendar.from_ical(event.to_ical())``

This fixes a problem when using the `decoded` method.
E.g., `event.decoded('dtstart')` fails with an exception:

```
  File ".../.pyenv/lib/python3.7/site-packages/icalendar/cal.py", line 237, in decoded
    return self._decode(name, value)
  File ".../.pyenv/lib/python3.7/site-packages/icalendar/cal.py", line 220, in _decode
    decoded = types_factory.from_ical(name, value)
  File ".../.pyenv/lib/python3.7/site-packages/icalendar/prop.py", line 1042, in from_ical
    decoded = type_class.from_ical(value)
  File ".../.pyenv/lib/python3.7/site-packages/icalendar/prop.py", line 334, in from_ical
    u = ical.upper()
```

This is due to a type inconsistency between the event created by this
package and the type that `icalendar` expects.

Another (ugly) workaround for this problem (if this patch was not
applied) would be to convert the event to ical and parse it back using
`icalendar`, which repairs the dtstart type inconsistency as well (e.g.,
something like `event = icalendar.Calendar.from_ical(event.to_ical())``
@wichmannpas
Copy link
Contributor Author

Consider this pull request rather as half an issue: I am not sure whether the tests are indeed supposed to force the specific date/datetime types (which do not seem to be used in that way by icalendar at that level). Feel free to edit the tests within this pull request (or find another solution to the issue).

@niccokunzmann
Copy link
Owner

@wichmannpas Thanks for reporting this - I did not encounter this so I am happy to see that you know about this. Can you add a source code - or best a test - which fails/breaks? Something that can be reproduced that is broken?

I am not sure whether the tests are indeed supposed to force the specific date/datetime types (which do not seem to be used in that way by icalendar at that level).
I added this because there are effects I cannot estimate: timezone/no timezone in the calendar, date/datetime, RDATE, EXRULE, ... So, I tested that if the event has a date as a start, the repetition also has a date and not a date+time.

@niccokunzmann
Copy link
Owner

To use these changes, the tests would also need to be changed. I did not really know about the vDDDType. I evaluate:

class vDDDTypes(object):
    """A combined Datetime, Date or Duration parser/generator. Their format
    cannot be confused, and often values can be of either types.
    So this is practical.
    """

https://github.com/collective/icalendar/blob/2b56bc4ee7e90fe7204174f5c8c4b48dc2de8636/src/icalendar/prop.py#L286

I think: If the tests can be changed this way: each event in the test is converted to bytes and back and then, the type is tested, then, they should work without further changes and this is quite nice and practical.

@niccokunzmann
Copy link
Owner

niccokunzmann commented Jan 3, 2020

What do you think?

  • note: unused method would also need to be removed.

@wichmannpas
Copy link
Contributor Author

wichmannpas commented Jan 3, 2020

Thanks for the feedback, I will try to have a look at the tests and add a test case for the problem in the next weeks.

@niccokunzmann niccokunzmann changed the title as_vevent: use vDDDTypes instead of explicit date/datetime type [WIP] as_vevent: use vDDDTypes instead of explicit date/datetime type Jan 3, 2020
niccokunzmann added a commit that referenced this pull request Jan 15, 2020
@niccokunzmann niccokunzmann merged commit c17cb23 into niccokunzmann:master Jan 15, 2020
@niccokunzmann
Copy link
Owner

@wichmannpas Thanks! I could remove some unnecessary tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants