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

fix: wrapping behaviour does no take added space into account #107

Merged
merged 1 commit into from
Aug 25, 2024

Conversation

marcantoinem
Copy link
Contributor

Thanks for this crates, it is really ergonomic to use, but I found a little uncommon bug:

In the section 3.1 of RFC 5545, the line size limit is stated to be no more than 75 bytes excluding the linebreak. This does not exclude the space character from the 75 bytes limit, so it needs to be taken into account in the folding algorithm. This PR fixes this bug and the corresponding test that assert the 76 bytes line.

@hoodie
Copy link
Owner

hoodie commented Aug 25, 2024

Thanks for the contribution, how did you run into this?

@marcantoinem
Copy link
Contributor Author

If you have a multiline that is bigger than 2 * 75 character the second line will not respect the standard. I was verifying with the icalendar verificator the generated ics to understand why Nextcloud Calendar didn't like it and I found the warning and dug deeper to understand the line size problem.

If you want to reproduces it just put a property like RDATE with a big list of dates and it will fail.

Also, the integration test before the PR is a example if you verify it.

Thanks for the quick response!

@hoodie
Copy link
Owner

hoodie commented Aug 25, 2024

Thank you for the contribution!!!
I would like to change the commit message a little bit before merging, since this ends up in the change log. Right now it would look like we're adding the bug :D

I would like to change it to

fix: take added space into account when wrapping lines

Up until now the folding did not correctly adhere to [RFC5545 3.1](https://datatracker.ietf.org/doc/html/rfc5545#section-3.1).

You could either allow me to push onto your branch or make the change yourself real quick. Thanks

@marcantoinem
Copy link
Contributor Author

I have amended the commit for that, is there a contributing guide to known how to name the commit to keep a consistent changelog?

@hoodie
Copy link
Owner

hoodie commented Aug 25, 2024

Thanks for the change. No unfortunately not yet. I should really add a contributing.md and a PR template here. Let's see if I can do that soon.

I'm trying to follow the conventional commits style in all my repos.

I'm sorry but could you please change the first line to fix: take added space into account when wrapping lines the second would be the description.

@marcantoinem
Copy link
Contributor Author

Ah, it is now a bit more clear for the formatting, I was not sure if the repo followed conventional commit.

@hoodie
Copy link
Owner

hoodie commented Aug 25, 2024

Thanks!!!

@hoodie hoodie merged commit b7b35c2 into hoodie:main Aug 25, 2024
7 checks passed
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.

2 participants