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

Default type vText when converting vRecur to ical #299

Merged
merged 2 commits into from Apr 8, 2020

Conversation

kam193
Copy link
Contributor

@kam193 kam193 commented Jan 15, 2020

Use default type vText when converting recurrence definition to ical string.

This is similar to from_ical behavior, that allows to set anything as recurrence key and treats it as vText; but at the same time, to_ical has strictly limited list of supported types and no default type, so the rule x == vDataType.from_ical(VDataType(x).to_ical()) (from https://github.com/collective/icalendar/blob/master/src/icalendar/prop.py#L33) is broken.

Furthermore, with this we can use non-standard recurrence keys (like BYEASTER supported by deteutil).

This is a fix without breaking changes.

@mister-roboto
Copy link

@kam193 thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@kam193
Copy link
Contributor Author

kam193 commented Jan 15, 2020

@jenkins-plone-org please run jobs

@kam193
Copy link
Contributor Author

kam193 commented Jan 15, 2020

It looks like broken CI on Plone and too long working hypothesis :/

@mauritsvanrees
Copy link
Member

I don't know enough (basically nothing) about icalendar. I just occasionally make a release. I cannot review this.

@mauritsvanrees mauritsvanrees removed their request for review January 17, 2020 15:17
@kam193 kam193 requested a review from thet January 20, 2020 15:53
@tuergeist
Copy link

@jenkins-plone-org please run jobs

1 similar comment
@kam193
Copy link
Contributor Author

kam193 commented Mar 6, 2020

@jenkins-plone-org please run jobs

Use default type when converting recurence definition
to ical string.
@kam193
Copy link
Contributor Author

kam193 commented Mar 18, 2020

@jenkins-plone-org please run jobs

@jensens
Copy link
Member

jensens commented Apr 5, 2020

Even I know not much about the internals of the package. But I would say vText as default makes sense. Out of curiosity: When is there no explicit value set? I would expect it to be there.

@kam193
Copy link
Contributor Author

kam193 commented Apr 5, 2020

The problem is when type is out of the standard range: icalendar library explicitly supports only limited number of types, but there is a lot of useful extending standard types supported by dateutil (the library used here to parse rrules).

Now, when we pass to icalendar a string with those non-standard types, it works (because from_ical has default type vText), but if we need this string again (this is my use case) there is an exception, because to_ical hasn't any default type :(

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't know much about this package. But seems okay. I will merge.
Thank you!

@mauritsvanrees mauritsvanrees merged commit c5f5602 into collective:master Apr 8, 2020
@kam193
Copy link
Contributor Author

kam193 commented Apr 8, 2020

Thanks!

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

5 participants