ENH: add support for calendar name #312

tupui · 2022-02-04T12:43:29Z

Closes #307

Add NAME and X-WR-CALNAME support.
X-WR-CALNAME is converted to NAME and X-WR-CALNAME is added in extras.
Default name is used when the field is not present.

(For the backstory, I talked about this with @stefanv)

tupui · 2022-02-04T12:46:27Z

Right now it fails to register. Although I can see that CalendarNameConverter.populate was called and modified component. It then fails when calling the AttributeValueConverter.post_populate. And I am stuck for now. Can anyone help?

    def post_populate(self, component: Component, context: ContextDict):
        if self.is_required and not context[(self, "current_value_count")]:
>           raise ValueError("attribute %s is required but got no value" % self.ics_name)
E           ValueError: attribute NAME is required but got no value

tupui · 2022-02-10T14:14:53Z

@N-Coder @C4ptainCrunch could you have a look? 😃

Also move timezone to root of yaml file This will eventually be supported through ics-py/ics-py#312, but this is a quick workaround in the interim.

C4ptainCrunch · 2022-02-13T20:32:23Z

Hi @tupui 👋

Sorry for the delay. First, thanks a lot for taking the time to submit a PR, you rock 🤘

I did not have a lot of time to look at your PR yet but i do have a question: Why do you add a default value for the calendar name ? Shouldn't we just keep it null if we did not get any data (and not writing a NAME property when serializing it back)

UPDATE: I had a closer look and fiddled a bit with your code. My changes are available in 8761dfc if you want. I hope it unsticks you :)

tupui · 2022-02-14T08:04:17Z

Hi @tupui 👋

Sorry for the delay. First, thanks a lot for taking the time to submit a PR, you rock 🤘

No worries!

I did not have a lot of time to look at your PR yet but i do have a question: Why do you add a default value for the calendar name ? Shouldn't we just keep it null if we did not get any data (and not writing a NAME property when serializing it back)

I thought a calendar should be named, but no problem to make this completely optional and not present if not specified.

UPDATE: I had a closer look and fiddled a bit with your code. My changes are available in 8761dfc if you want. I hope it unsticks you :)

Oh thanks! I will apply your changes then 😃

tupui · 2022-02-14T08:13:03Z

src/ics/converter/types/calendar.py

@@ -62,4 +63,24 @@ def serialize(self, component: Component, output: Container, context: ContextDic
        context["VTIMEZONES_AFTER"] = len(output)


+class CalendarNameConverter(GenericConverter):


~~I suppose we can then remove all changes to this file then?~~ Actually this still required for X-WR-CALNAME rewrite to point to NAME.

Sorry I did not get the optional validator. I had trouble to understand how these were working together.

I suppose we can then remove all changes to this file then?

yes, please do so

But then we don't have X-WR-CALNAME at all if we do this. Shouldn't we try to always add X-WR-CALNAME to have both fields?

To me what is missing here is just adding NAME to an extra field X-WR-CALNAME

I would have this in CalendarNameConverter.populate

if item.name == "NAME": component.extra_params["X-WR-CALNAME"] = item.value return True

I can make the tests pass, but I am probably not doing it right, you're probably better of doing this yourself at this point 😄

tupui · 2022-02-14T08:30:06Z

tests/test_calendar.py

+    # FIXME: what behavior should we expect here?
+    # FIXME: In both cases, we need a test and we need to document it.


Since the RFC is only referring to NAME, I suppose one could say that if both fields are present, it should take NAME instead of X-WR-CALNAME. This is what I've done now.

C4ptainCrunch · 2022-02-16T16:27:18Z

Great ! This looks in good shape to be merged 🎉

But first, could you :

Add yourself to AUTHORS.rst
Reference this change in CHANGELOG.rst
Add documentation about the feature (and insist on the NAME vs X-WR-CALNAME precedence as well as the behaviour when X-WR-CALNAME is in the input and we serialize the output with NAME)

src/ics/converter/types/calendar.py

tupui · 2022-02-16T16:36:10Z

Thanks @C4ptainCrunch! Where do you want me do document this? I am not sure as to where this would fit better.

N-Coder

Thanks a lot for making the effort to get your way around my Converter system and creating this PR - and sorry for taking my time to reply! Unfortunately, your code doesn't quite do what you intended, see my reviews. I see this as another argument for the Converter system being in desperate need of some refactoring and especially bigger simplifications. I guess the simplest way forward here would be using the following implementation for your propagate, adding handling of conflicting specifications later on once the Converter system makes that easier to do:

     def populate(self, component: Component, item: ContainerItem, context: ContextDict) -> bool:
            component.name = item.value
            return False

One thing that should be noted: with this PR, the library will now throw an exception when it sees multiple values for name, e.g. because they were localized, while before the multiple values could be stored to the extra properties just fine. This the second big "construction site", where the library is too strict in many cases. But I guess supporting one one name for now is okay.

src/ics/converter/types/calendar.py

src/ics/icalendar.py

tupui

Thanks for the review @N-Coder!

As I said bellow, I just came across the fact that we would need X-WR-CALNAME for practical reason.

@C4ptainCrunch @N-Coder Let me know what should be the behaviour. I personally would go with providing both fields (either another attribute or in extra) and NAME would take precedence over X-WR-CALNAME to follow a bit more the RFC.

src/ics/converter/types/calendar.py

src/ics/icalendar.py

N-Coder · 2022-03-04T08:22:36Z

As I said bellow, I just came across the fact that we would need X-WR-CALNAME for practical reason.

I guess the best way to support this would be parsing both values into the same field name, and allowing the user to select a compatibility level for serialization (i.e. RFC5545, RFC7986, Google, Outlook, ...) that would then decide whether the value is written out as NAME or X-WR-CALNAME.

Unfortunately, we don't have that yet and it only adds to the complexity of refactoring the (de-)serialization system, so it will probably also take a while. In the meantime, I'd say the sanest thing to do is add a name field that only maps to NAME in the ics and let X-WR-CALNAME go entirely separate via the extras. Adding a comment to the name field about Google and X-WR-CALNAME and not doing any incomplete translation between both properties would then probably cause the least suprises for users.

tupui · 2022-03-08T09:35:32Z

Unfortunately, we don't have that yet and it only adds to the complexity of refactoring the (de-)serialization system, so it will probably also take a while. In the meantime, I'd say the sanest thing to do is add a name field that only maps to NAME in the ics and let X-WR-CALNAME go entirely separate via the extras. Adding a comment to the name field about Google and X-WR-CALNAME and not doing any incomplete translation between both properties would then probably cause the least suprises for users.

@N-Coder This is the current behaviour implemented in this PR no?

N-Coder · 2022-03-08T10:21:36Z

If you remove the added CalendarNameConverter by reverting the changes to src/ics/converter/types/calendar.py that should be it. Then we simply add the name field and letting its Converter be autogenerated without ever touching the X-WR-CALNAME.

src/ics/icalendar.py

C4ptainCrunch · 2022-07-15T11:01:17Z

@tupui: are you still interested in working on this PR ? Could you have a look at the failing test ? :)

tupui · 2022-07-15T15:07:03Z

I am interested but as you've seen, you or @N-Coder would just be faster finishing this up vs discussing what I should do. I am afraid I don't have much time to focus on the architecture of the package to make this right.

C4ptainCrunch · 2022-12-15T03:02:43Z

I'm closing this due to the previous comment. Anybody can reopen if they have time to work on this :)

stefanv · 2022-12-15T12:56:36Z

This felt like an important issue. We can work around it, as we've been doing, but calendars without names are much less useful for importing on eg Google Calendar. If the PR is closed, shouldn't an issue at least be opened?

C4ptainCrunch · 2022-12-18T20:11:03Z

@stefanv You are totally right ! I opened #375 to track this :)

ENH: add support for calendar name

7ce81b7

stefanv added a commit to scientific-python/yaml2ics that referenced this pull request Feb 11, 2022

Add calendar name field

8fd29c2

Also move timezone to root of yaml file This will eventually be supported through ics-py/ics-py#312, but this is a quick workaround in the interim.

stefanv mentioned this pull request Feb 11, 2022

Add calendar name field scientific-python/yaml2ics#13

Merged

stefanv added a commit to scientific-python/yaml2ics that referenced this pull request Feb 11, 2022

Add calendar name field

5a44a63

Also move timezone to root of yaml file This will eventually be supported through ics-py/ics-py#312, but this is a quick workaround in the interim.

stefanv added a commit to scientific-python/yaml2ics that referenced this pull request Feb 11, 2022

Add calendar name field (#13)

6dbc75b

Also move timezone to root of yaml file This will eventually be supported through ics-py/ics-py#312, but this is a quick workaround in the interim.

Merge branch 'main' into name_calendar

2f60393

Remove required NAME and superfluous code + add a few tests

bafba59

tupui marked this pull request as ready for review February 14, 2022 08:10

tupui commented Feb 14, 2022

View reviewed changes

Add name precedence over x-wr-calname

258a97c

tupui commented Feb 14, 2022

View reviewed changes

Clean tests

6646082

C4ptainCrunch reviewed Feb 16, 2022

View reviewed changes

src/ics/converter/types/calendar.py Show resolved Hide resolved

tupui added 2 commits February 16, 2022 17:40

Update changelog and authors

2378335

Improve docstring with X-WR-CALNAME, NAME logic

e011cb2

N-Coder requested changes Feb 16, 2022

View reviewed changes

src/ics/converter/types/calendar.py Show resolved Hide resolved

src/ics/converter/types/calendar.py Show resolved Hide resolved

src/ics/converter/types/calendar.py Show resolved Hide resolved

src/ics/icalendar.py Outdated Show resolved Hide resolved

Change priority for name

623cc2d

tupui commented Feb 17, 2022

View reviewed changes

src/ics/converter/types/calendar.py Show resolved Hide resolved

src/ics/icalendar.py Outdated Show resolved Hide resolved

tupui requested review from N-Coder and C4ptainCrunch February 17, 2022 20:42

N-Coder mentioned this pull request Mar 4, 2022

Roadmap for v0.8 #245

Open

21 tasks

N-Coder reviewed Mar 8, 2022

View reviewed changes

src/ics/icalendar.py Outdated Show resolved Hide resolved

Update priority CalendarTimezoneConverter

0b3946b

tupui force-pushed the name_calendar branch from 405892a to 0b3946b Compare March 8, 2022 10:30

X-WR-CALNAME always in extras

80b7245

C4ptainCrunch requested a review from N-Coder July 4, 2022 13:44

C4ptainCrunch added the merge-before-black We should merge this PR before running Black label Jul 4, 2022

Merge branch 'main' into name_calendar

3852300

Merge branch 'main' into name_calendar

1ed3a07

C4ptainCrunch removed the merge-before-black We should merge this PR before running Black label Jul 22, 2022

C4ptainCrunch closed this Dec 15, 2022

C4ptainCrunch mentioned this pull request Dec 18, 2022

Add support for calendar name (RFC7986) and/or X-WR-CALNAME #375

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add support for calendar name #312

ENH: add support for calendar name #312

tupui commented Feb 4, 2022 •

edited

tupui commented Feb 4, 2022

tupui commented Feb 10, 2022

C4ptainCrunch commented Feb 13, 2022 •

edited

tupui commented Feb 14, 2022

tupui Feb 14, 2022 •

edited

N-Coder Mar 8, 2022

tupui Mar 8, 2022 •

edited

tupui Mar 8, 2022 •

edited

tupui Mar 8, 2022

tupui Feb 14, 2022

C4ptainCrunch commented Feb 16, 2022

tupui commented Feb 16, 2022

N-Coder left a comment

tupui left a comment •

edited

N-Coder commented Mar 4, 2022

tupui commented Mar 8, 2022 •

edited

N-Coder commented Mar 8, 2022

C4ptainCrunch commented Jul 15, 2022

tupui commented Jul 15, 2022

C4ptainCrunch commented Dec 15, 2022

stefanv commented Dec 15, 2022

C4ptainCrunch commented Dec 18, 2022

		@@ -62,4 +63,24 @@ def serialize(self, component: Component, output: Container, context: ContextDic
		context["VTIMEZONES_AFTER"] = len(output)


		class CalendarNameConverter(GenericConverter):

		# FIXME: what behavior should we expect here?
		# FIXME: In both cases, we need a test and we need to document it.

ENH: add support for calendar name #312

ENH: add support for calendar name #312

Conversation

tupui commented Feb 4, 2022 • edited

tupui commented Feb 4, 2022

tupui commented Feb 10, 2022

C4ptainCrunch commented Feb 13, 2022 • edited

tupui commented Feb 14, 2022

tupui Feb 14, 2022 • edited

Choose a reason for hiding this comment

N-Coder Mar 8, 2022

Choose a reason for hiding this comment

tupui Mar 8, 2022 • edited

Choose a reason for hiding this comment

tupui Mar 8, 2022 • edited

Choose a reason for hiding this comment

tupui Mar 8, 2022

Choose a reason for hiding this comment

tupui Feb 14, 2022

Choose a reason for hiding this comment

C4ptainCrunch commented Feb 16, 2022

tupui commented Feb 16, 2022

N-Coder left a comment

Choose a reason for hiding this comment

tupui left a comment • edited

Choose a reason for hiding this comment

N-Coder commented Mar 4, 2022

tupui commented Mar 8, 2022 • edited

N-Coder commented Mar 8, 2022

C4ptainCrunch commented Jul 15, 2022

tupui commented Jul 15, 2022

C4ptainCrunch commented Dec 15, 2022

stefanv commented Dec 15, 2022

C4ptainCrunch commented Dec 18, 2022

tupui commented Feb 4, 2022 •

edited

C4ptainCrunch commented Feb 13, 2022 •

edited

tupui Feb 14, 2022 •

edited

tupui Mar 8, 2022 •

edited

tupui Mar 8, 2022 •

edited

tupui left a comment •

edited

tupui commented Mar 8, 2022 •

edited