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

Adding line folding #5

Closed
wants to merge 1 commit into from
Closed

Conversation

Dynom
Copy link

@Dynom Dynom commented Jan 7, 2021

Following the recommendation, I've added line-folding with multi-byte character support.

Feel free to fire away review feedback :-)

Couple of notes:

  • The generated ICS validates on: https://icalendar.org/validator.html
  • The spec shows N+1 indent per line, but I think the specification is somewhat ambiguous and I've found real-world examples who continue with just 1 indent per subsequent line.
  • This PR introduces a dependency on at least Go 1.10

@emersion
Copy link
Owner

emersion commented Jan 8, 2021

Thanks for sending a PR. Have you noticed that not folding lines breaks some calendaring software?

In my experience, line folding is hard to get right and has been the cause of many bugs. So I'd rather not add it if everything works fine without it.

@Dynom
Copy link
Author

Dynom commented Jan 11, 2021

Hi, and thanks for your reply.

I haven't had a problem with long line lengths just yet.

However in our case, which might be a niche one, we allow for quite a bit of content in the description field. In this case I've followed a similar approach as to how Google generate's their ICS files and so far it validates and is accepted by the clients I've tested (macOS, Outlook and gmail).

Would you consider making this an opt-in feature? I could change the PR to feature-toggle it.

The approach I'd be considering would be to introduce functional options, to keep BC:

Changing:

func NewEncoder(w io.Writer, options ...Option) *Encoder {
  enc := Encoder{w:w}
  for _, o := range options {
    o(&enc)
  }

  return &enc
}

Introducing:

type Option func(enc *Encoder)
func WithLineFolding() Option {
  return func(enc *Encoder) {
    enc.maxLineLength = 75
  }
}

Using:

// Backwards compatability
enc := ical.NewEncoder(w)

// With line-folding
encWithLineFolding := ical.NewEncoder(w, WithLineFolding())

Changing line folding on the encoder isn't really a problem, since it's marshalling logic, but this might be a clean API to have a clear intent from the start.

@Dynom
Copy link
Author

Dynom commented Jan 11, 2021

In my experience, line folding is hard to get right and has been the cause of many bugs. So I'd rather not add it if everything works fine without it.

To learn from your experiences, what kind of bugs did you encounter with line folding?

@emersion
Copy link
Owner

From a quick look, here are a few issues I've hit:

emersion/go-msgauth#18
emersion/go-msgauth#23
emersion/go-msgauth#36
emersion/go-message#54
emersion/go-message#44
emersion/go-message#22

Would you consider making this an opt-in feature?

No.

we allow for quite a bit of content in the description field. In this case I've followed a similar approach as to how Google generate's their ICS files and so far it validates and is accepted by the clients I've tested (macOS, Outlook and gmail).

Why is folding necessary for long description values?

@Dynom
Copy link
Author

Dynom commented Jan 11, 2021

From a quick look, here are a few issues I've hit:

emersion/go-msgauth#18
emersion/go-msgauth#23
emersion/go-msgauth#36
emersion/go-message#54
emersion/go-message#44
emersion/go-message#22

I understand that a bad experience from line-folding in general can be discouraging, E-mail headers and DKIM in particular can be a bit finicky.

we allow for quite a bit of content in the description field. In this case I've followed a similar approach as to how Google generate's their ICS files and so far it validates and is accepted by the clients I've tested (macOS, Outlook and gmail).

Why is folding necessary for long description values?

It's not. The specification mentions that they should, not that they must be limited to 75 bytes.

I was porting an implementation from a different language and the difference was, besides the key-sorting go-ical does, on the line lengths.

I'll close the PR as you've made your point clear on the matter.

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