Strong types, not ignoring the VALUE parameter, better error handling #196

stlaz · 2016-08-12T13:34:09Z

Hello,
Long time no patch from me so I decided it might be good to finally improve error handling. I built the error handling improvements on top of #192 as I believe this patch may finally get it pushed.

stlaz · 2016-08-16T09:35:34Z

I will leave running your Jenkins jobs on you if you don't mind.

gforcada · 2016-08-16T09:45:47Z

@stlaz I just did (starting the jenkins jobs), you can find a guide on how to do that yourself next time here

stlaz · 2016-08-18T11:18:16Z

It would seem those tests failed if I see the right jobs.
http://jenkins.plone.org/job/pull-request-5.0/1349/
http://jenkins.plone.org/job/pull-request-5.1/536/
Do you know what causes the failure, @gforcada? The tests in python-icalendar, the only repository I really care about, pass. If those tests that do not pass are valid could you then add them to this repository instead?

Also, there should probably be better notification of test failure in the Pull Request report.

stlaz · 2016-08-30T07:37:45Z

@gforcada I believe the Internet revealed the string that should have made the test fail which is the one in test_portal_ical on Plone:tests.test_icalendar/. However, it does not seem to fail for me.
@geier or @thet, do you guys think you could look into it and possibly (N)ACK these changes for pushing? Thanks 👍

geier · 2016-09-07T22:13:31Z

src/icalendar/prop.py

-    @staticmethod
-    def from_ical(ical, timezone=None):
+    @classmethod
+    def from_ical(cls, ical, unit_type=None, timezone=None):


I think we should keep the old order of arguments, this breaks backwards compatibility otherwise.

You're right, that wasn't clever at all, don't know why I did that. Should be fixed now.

stlaz · 2016-09-08T08:37:28Z

@gforcada I fixed the issue pointed out by @geier in a comment and decided to go ahead and run a Jenkins job according to the guide. Allowed the application to read whatever it wanted to read and I suddenly get "stlaz is missing the Overall/Read permission" on http://jenkins.plone.org/. This seems a bit funny to me because I have more read permissions as an anonymous user (when I'm not logged in).

gforcada · 2016-09-08T12:34:49Z

@stlaz probably that's related to you not being in github.com/plone organization I think... I will run the jobs for you

gforcada · 2016-09-08T12:39:49Z

http://jenkins.plone.org/job/pull-request-5.0/1410/
http://jenkins.plone.org/job/pull-request-5.1/602/

stlaz · 2016-09-09T11:44:40Z

Still not sure why the IcalendarTestExportDX tests are failing, I was thinking they would be caused by the backward compatibility issues which I was hoping I fixed.
I shall look more into the decoded() method, I think possibly there might be bug in it not checking the VALUE parameter. However, if you look into the exception, you see that it's actually correct, '20130404' is indeed wrong datetime format.

geier · 2016-09-10T11:12:50Z

I've created https://github.com/geier/icalendar/tree/fix_failures to help pinpoint those errors.

geier · 2016-09-10T11:13:07Z

src/icalendar/prop.py

@@ -402,6 +429,10 @@ def from_ical(ical, timezone=None):
                tzinfo = _timezone_cache.get(timezone, None)

        try:
+            if ical[8] != 'T':


check for length here

ignore that

I will swap this check with the length check to make it a bit cleaner.

stlaz · 2016-09-13T08:44:15Z

Thanks @geier, that helped! I was able to find the issues with the decoded() method. Still not able to reproduce the IcalendarTestExportDX issue.
Btw: There's an error in those tests you uploaded. The error is on lines 63, 64 of plone.app.event.icaltest.ics and on lines 61, 62 of src/icalendar/tests/plone.app.event.icaltest_2.ics. The 'DTSTART' and 'DTEND' property type defaults to 'DATE-TIME' in RFC 5545 (not sure if this is a change to the previous version) therefore 'VALUE=DATE' has to be defined.

thet · 2016-10-26T06:03:08Z

@gforcada @stlaz I think people contributing to icalendar shouldn't deal with Plone's CI suite.
icalendar is used quite a lot out of the Plone context. Passing travis tests is all what's required.

@gforcada what is needed to remove the Plone CI integration?

stlaz · 2016-10-26T11:46:05Z

@thet: This time it helped to reveal a bug so I was happy we had it there :) But you, Plone, can always run those tests yourself and open an Issue if something is broken by my patches, mark me as an Assignee and I shall have a look at it. That seems a bit more reasonable than having CI tests that the community can't run anyway.

geier · 2016-10-26T12:39:05Z

@stlaz sorry, somehow I didn't see your replies, I know these the test data isn't RFC5545 compliant, but it's what the plone people use, so I'm sure they wouldn't appreciate it if we broke this in a non-major release.

And yes, this is somewhat inconsistent with the behavior of icalendar in other places, like only accepting timeshifts for VTIMEZONE components <= 24h. I'm actually in favor of some kind of strict or lenient mode for icalendar.

stlaz · 2016-10-27T11:06:07Z

@geier If I recall correctly, originally this patch enabled lazier approach to parsing iCalendar strings, leaving strange values at the given property as strings, while also keeping the property parameters. This should allow the user of the parser to evaluate the value according to their needs instead of parsing it in a wrong way and possibly dropping some of the parameters on the way.
That being said, wouldn't it be time to get the major version features prepared so that new major version could be released? :)

geier · 2016-10-27T15:51:45Z

That being said, wouldn't it be time to get the major version features prepared so that new major version could be released? :)

I fully agree, but at the moment I do not have a lot of time for this.

collective#187

Previously, if error occured during the creation of inner type instance from iCalendar string, the error with the name of the bogus property would be stored in the appropriate component's errors attribute along with the error string but the property's value would be removed from the parsed representation. This patch keeps the value even with its parameters at that certain property, the value's type is changed to vText. Should allow implementation of collective#158 Improves collective#174

geier · 2017-07-24T16:24:22Z

src/icalendar/prop.py

        for dt in dt_list:
-            dt = vDDDTypes(dt)
+            # raise ValueError if type of the input values differs
+            if not isinstance(dt, ltype):


Wouldn't this test fail if the first element in the list is a date and the later ones are datetime? I believe we will need to actually do a if type(dt) != type(ltype) here.

Hm, you're right, it would seem that we need to be even more strict here. Thanks for pointing that out with the date/datetime example, I did not realize.

geier · 2017-07-24T16:56:50Z

I have used khal's testsuite as an additional test for this PR and while I have found an issue, minimal example as follows:

import icalendar
import datetime as dt
event = icalendar.Event()                                                                                                                                                        
event.add('DTSTART', dt.date.today())                                                                                                                                            
event.to_ical()

leads to an AttributeError:

  File "<stdin>", line 1, in <module>
  File "/usr/home/cg/workspace/icalendar/src/icalendar/cal.py", line 450, in to_ical
    content_lines = self.content_lines(sorted=sorted)
  File "/usr/home/cg/workspace/icalendar/src/icalendar/cal.py", line 439, in content_lines
    cl = self.content_line(name, value, sorted=sorted)
  File "/usr/home/cg/workspace/icalendar/src/icalendar/cal.py", line 432, in content_line
    return Contentline.from_parts(name, params, value, sorted=sorted)
  File "/usr/home/cg/workspace/icalendar/src/icalendar/parser.py", line 304, in from_parts
    values = values.to_ical()
  File "/usr/home/cg/workspace/icalendar/src/icalendar/prop.py", line 404, in to_ical
    tzid = tzid_from_dt(dt)
  File "/usr/home/cg/workspace/icalendar/src/icalendar/parser.py", line 52, in tzid_from_dt
    if hasattr(dt.tzinfo, 'zone'):
AttributeError: 'datetime.date' object has no attribute 'tzinfo'

geier · 2017-07-24T17:04:25Z

Documentation issue: accessing a DURATION's timedelta changed from event['DURATION'].dt to event['DURATION'].td.

stlaz · 2017-07-26T11:01:07Z

The documentation issue did not appear in this PR for the first time though, did it?

geier · 2017-07-26T12:07:49Z

I believe it does, the following snippet works on master but not with this PR:

import icalendar
import datetime as dt
event = icalendar.Event()
event.add('DURATION', dt.timedelta(hours=2))
print(event['DURATION'].dt)

stlaz · 2017-08-15T06:45:46Z

Hello,
Sorry I did not update the PR recently, I was on a vacation away from my laptop. I have some work to do this week but i hope I'll get to this by the end of it.

I believe the dt -> td doc issue is caused by the duration property actually correctly resolving to the vDuration type so it's indeed just a doc issue.

I need to look more closely on the first issue though. It's apparent that datetime.date object would not have tzinfo attribute, I'll just need to see how much we should care :)

stlaz · 2017-11-03T12:48:29Z

I am sorry I did not get enough time for this for the past few months, a lot happened. I should hope to get to this real soon, hopefully.

A note to the failure with the simple example event.add('DTSTART', dt.date.today()). This will create an invalid event since DTSTART expects DATE-TIME value by default. This should be handled more properly during to_ical I guess.

thet · 2021-10-13T09:56:32Z

Revisiting this after more than 5 years!
I resolved the last mentioned issues from the comments in: #331
Thanks @stlaz for your work and @geier for the in-depth review!

I think we can get #331 merged before the RFC5545 standard or computers at all eventually becomes obsolete.

stlaz changed the title ~~Error handling~~ Improved error handling Aug 12, 2016

stlaz mentioned this pull request Aug 26, 2016

Time-Based HBAC Policies freeipa/freeipa#23

Closed

geier reviewed Sep 7, 2016
View reviewed changes

stlaz force-pushed the error_handling branch from c01d381 to 9c5de10 Compare September 8, 2016 07:53

geier reviewed Sep 10, 2016
View reviewed changes

stlaz force-pushed the error_handling branch from 9c5de10 to b257aa5 Compare September 13, 2016 08:35

stlaz mentioned this pull request Apr 21, 2017

If not strict, don't break on UTCOffsets > 24 hours. #222

Closed

stlaz added 2 commits July 20, 2017 09:01

Introducing strong types to python-icalendar

44692cb

collective#187

stlaz force-pushed the error_handling branch from b257aa5 to 43bdc34 Compare July 20, 2017 07:06

stlaz changed the base branch from master to devel-4.0 July 20, 2017 07:07

stlaz changed the title ~~Improved error handling~~ Strong types, not ignoring the VALUE parameter, better error handling Jul 20, 2017

stlaz mentioned this pull request Jul 20, 2017

Strong types + no longer ignore the VALUE parameter #192

Closed

geier reviewed Jul 24, 2017

View reviewed changes

thet added a commit that referenced this pull request Oct 13, 2021

Fix DTSTART failed with date. See: #196 (comment)

deb6919

thet mentioned this pull request Oct 13, 2021

Strong types, not ignoring the VALUE parameter, better error handling #331

Open

thet closed this Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strong types, not ignoring the VALUE parameter, better error handling #196

Strong types, not ignoring the VALUE parameter, better error handling #196

stlaz commented Aug 12, 2016

stlaz commented Aug 16, 2016

gforcada commented Aug 16, 2016

stlaz commented Aug 18, 2016 •

edited

Loading

stlaz commented Aug 30, 2016

geier Sep 7, 2016

stlaz Sep 8, 2016

stlaz commented Sep 8, 2016

gforcada commented Sep 8, 2016

gforcada commented Sep 8, 2016

stlaz commented Sep 9, 2016

geier commented Sep 10, 2016

geier Sep 10, 2016 •

edited

Loading

stlaz Sep 13, 2016

stlaz commented Sep 13, 2016

thet commented Oct 26, 2016

stlaz commented Oct 26, 2016

geier commented Oct 26, 2016

stlaz commented Oct 27, 2016

geier commented Oct 27, 2016

geier Jul 24, 2017

stlaz Jul 26, 2017

geier commented Jul 24, 2017 •

edited

Loading

geier commented Jul 24, 2017

stlaz commented Jul 26, 2017 •

edited

Loading

geier commented Jul 26, 2017

stlaz commented Aug 15, 2017

stlaz commented Nov 3, 2017

thet commented Oct 13, 2021

Strong types, not ignoring the VALUE parameter, better error handling #196

Strong types, not ignoring the VALUE parameter, better error handling #196

Conversation

stlaz commented Aug 12, 2016

stlaz commented Aug 16, 2016

gforcada commented Aug 16, 2016

stlaz commented Aug 18, 2016 • edited Loading

stlaz commented Aug 30, 2016

geier Sep 7, 2016

Choose a reason for hiding this comment

stlaz Sep 8, 2016

Choose a reason for hiding this comment

stlaz commented Sep 8, 2016

gforcada commented Sep 8, 2016

gforcada commented Sep 8, 2016

stlaz commented Sep 9, 2016

geier commented Sep 10, 2016

geier Sep 10, 2016 • edited Loading

Choose a reason for hiding this comment

stlaz Sep 13, 2016

Choose a reason for hiding this comment

stlaz commented Sep 13, 2016

thet commented Oct 26, 2016

stlaz commented Oct 26, 2016

geier commented Oct 26, 2016

stlaz commented Oct 27, 2016

geier commented Oct 27, 2016

geier Jul 24, 2017

Choose a reason for hiding this comment

stlaz Jul 26, 2017

Choose a reason for hiding this comment

geier commented Jul 24, 2017 • edited Loading

geier commented Jul 24, 2017

stlaz commented Jul 26, 2017 • edited Loading

geier commented Jul 26, 2017

stlaz commented Aug 15, 2017

stlaz commented Nov 3, 2017

thet commented Oct 13, 2021

stlaz commented Aug 18, 2016 •

edited

Loading

geier Sep 10, 2016 •

edited

Loading

geier commented Jul 24, 2017 •

edited

Loading

stlaz commented Jul 26, 2017 •

edited

Loading