search

Peltoche / ical-rs

Rust parser for ics (rfc5545) and vcard (rfc6350)
Apache License 2.0
110 stars 21 forks source link

Fix newlines #56

Closed westy92 closed 10 months ago

westy92 commented 1 year ago

Per the spec, lines should end with \r\n, not just \n.

4.1 Content Lines

The iCalendar object is organized into individual lines of text, called content lines. Content lines are delimited by a line break, which is a CRLF sequence (US-ASCII decimal 13, followed by US-ASCII decimal 10).

https://www.ietf.org/rfc/rfc2445.txt

Peltoche commented 1 year ago

Hello @westy92, thanks for this PR.

It seems that you are right about the specs but, as you can see with the tests, you change is not retro-compatible. I have only supported the "classic newline" (\n) because all the vcard/vcal I have found was using it.

Could you change your PR to make it able to handle both \n and \r\n please?

westy92 commented 1 year ago

@Peltoche thanks for the feedback, I'll try to get this fixed ASAP. Everything initially passed locally - it looks like I need to pass --all-features to run all tests and see these failures.

westy92 commented 10 months ago

@Peltoche all tests are now passing!

Peltoche commented 10 months ago

Hello @westy92, happy new year and sorry for the delay. I have a lot of stuff going on in my private life and this lib is really not the first of my priorities. But we will succeed to merge this PR soon I hope!

I only have one issue with your PR, you change the previous tests and this indicate that you are creating a change in behaviour. I don't think we need or should create a breaking change to solve you issue. If I handled only the \n and nobody have raised and issue before is because the parser is based on a lot of real case scenarios which use \n as newline indicator.

So could you change the code to handle both \n and \r\n` as a newline indicator please? This should avoid to change the current behaviour and allows us to have to correct behaviour.

Thanks again for you contribution.

westy92 commented 10 months ago

Hi @Peltoche.

Thank you for the additional PR feedback.

The parsing code is unchanged, along with its unit tests. The only code that is different is the generator.

I added a unit test with a spec-compliant ICS file to prove this.

If anything, this is a bug fix to bring ICS generated by the library into compliance.

Peltoche commented 10 months ago

Indeed, I have done the review too quickly. I merge it. Thanks for the contrib :+1:

westy92 commented 10 months ago

Thank you for the approval, merge, and release! 🎉