这是indexloc提供的服务,不要输入任何密码
Skip to content

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