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

string conversion of summary field converts "," to "\," #127

Open
schaefer0 opened this issue Feb 4, 2014 · 5 comments
Open

string conversion of summary field converts "," to "\," #127

schaefer0 opened this issue Feb 4, 2014 · 5 comments

Comments

@schaefer0
Copy link

when the calendar event summary field contains a string with a comma, for example
"key=value1,value2"
the to_ical() function converts the above string to: "key=value1,value2"

Is this a bug or a feature?

thanks - bob s.

@untitaker
Copy link
Contributor

Skimming over the iCalendar RFC it seems to be a feature, at least for value type TEXT. But it is entirely dependent on the key you're having!

@pdav
Copy link

pdav commented May 22, 2016

In src/icalendar/prop.py, the 'categories' entry should not have type 'text' (in types_map array) since (quoting @geier in pimutils/khal#414):

in the RFC the example for multiple categories (CATEGORIES:APPOINTMENT,EDUCATION) is comma separated, while the current implementation here escapes the comma with a backslash (example: CATEGORIES:CatA\,CatB)

More specifically, method vText.to_ical() (in src/icalendar/prop.py) calls escape_char (in src/icalendar/parser.py) which escapes ",".

I guess a new type (list ?) should be introduced.

@stlaz
Copy link
Collaborator

stlaz commented May 27, 2016

You can read from the quoted RFC that the 'text' type in 'categories' entry is actually right. There's no type "list" in iCalendar, there's just multiple values, but that's not applicable to TEXT values.
As for escaping the commas, it seems like a bug to me. If the from_ical method gets text with unescaped "," it should not try to escape it.

@pdav
Copy link

pdav commented May 29, 2016

The quoted RFC is ambiguous about the type TEXT since this type may be both a single value or a list.

Quoting the TEXT specification: If the property permits, multiple TEXT values are specified by a COMMA-separated list of values.

Thus, some properties (e.g. SUMMARY) are single TEXT with the requirement to escape the comma, and some properties are multiple TEXT values (e.g. CATEGORIES) which need comma-quoting on single values and not on the comma used as a separator.

From my point of view, icalendar should either:

  • treat them differently in the API (the text icalendar type cannot be used for both),
  • or leave the responsability of escaping the comma to the calling application, and never escape the comma in to_ical/from_ical

@stlaz
Copy link
Collaborator

stlaz commented May 29, 2016

Ah, sorry, you're right, I did not notice that and it actually makes sense why the comma should be escaped in TEXT, then. The value type of CATEGORIES is still TEXT, though.
I believe that with #192, the vDDDLists class may actually be able to handle vText lists although some changes in the TypesFactory will probably be required.
The parsing of TEXT values would still need to be fixed first as "," != "," on the input of from_ical methods.

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

No branches or pull requests

4 participants