arran4 / golang-ical

A ICS / ICal parser and serialiser for Golang.
Apache License 2.0
297 stars 73 forks source link

Fix #48: removing newlines inside a property #50

Closed JM-Lemmi closed 2 years ago

JM-Lemmi commented 2 years ago

by intercepting iostream in NewCalendar and removing 0x0d 0x0a 0x09

arran4 commented 2 years ago

Thanks @JM-Lemmi I'm a little skeptical about this PR.

Can you a) Produce a test that will only succeed with this test b) Provide a good example to why this is desirable. Examples etc? c) Can in be done in parser, without reading the entire stream and recreating it? (Ie you could wrap the io reader with your own stripping reader) d) Ensure that the error from io.ReadAll isn't ignored.

JM-Lemmi commented 2 years ago

Do you want it moved into the NewCalendarStream function?

arran4 commented 2 years ago

@JM-Lemmi Thanks for adding the tests. -- I will get to this in a bit. However I don't think the solution is the best as it might possibly impact other aspects of the app. I will use your tests (and existing code as guidance) in a fix. Unless you want to take another stab at the solution?

arran4 commented 2 years ago

(However I will have to do it when I have time as this is unpaid.)

JM-Lemmi commented 2 years ago

Feel free to implement this your way, I won't hold a grudge. I just wanted something that works fast, since I am using the module in another project and this seemed the simplest solution.

arran4 commented 2 years ago

Understood.

arran4 commented 2 years ago

Hi @JM-Lemmi

Your tests don't fail when I run them against current master: image

Copied files in with:

$ curl https://raw.githubusercontent.com/arran4/golang-ical/0c0d52328710044004a15aeb36a6b9af540628e0/testdata/google.ics > google.ics
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   754  100   754    0     0  10328      0 --:--:-- --:--:-- --:--:-- 10472
$ curl https://raw.githubusercontent.com/arran4/golang-ical/0c0d52328710044004a15aeb36a6b9af540628e0/testdata/outlook.ics > outlook.ics
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1416  100  1416    0     0   4985      0 --:--:-- --:--:-- --:--:--  4985

Can you please provide files which fail on master but should pass?

arran4 commented 2 years ago

See https://github.com/arran4/golang-ical/pull/54/files

JM-Lemmi commented 2 years ago

Your tests don't fail when I run them against current master

Indeed. It works on master on my end now too. I'll have to see later if I can reproduce the errors or what changed.

JM-Lemmi commented 2 years ago

Ah I see. Your commit e3ae829 addressed the issue already.

$ git checkout e3ae8290e7b88fbc7631d488e6211ce4f8f2d6cb
M       calendar_test.go
Previous HEAD position was 4cfb548 Merge pull request #47 from fty4/feature/duration
HEAD is now at e3ae829 4.1 Content Lines
$ go test
PASS
ok      github.com/arran4/golang-ical   0.004s
$ git checkout 4cfb54863a7921c9f34d8c67634275d8766a652b
M       calendar_test.go
Previous HEAD position was e3ae829 4.1 Content Lines
HEAD is now at 4cfb548 Merge pull request #47 from fty4/feature/duration
$ go test
--- FAIL: TestLineFoldingInput (0.00s)
    calendar_test.go:180: 
                Error Trace:    calendar_test.go:180
                Error:          Expected nil, but got: &errors.errorString{s:"Malformed calendar"}
                Test:           TestLineFoldingInput
FAIL
exit status 1
FAIL    github.com/arran4/golang-ical   0.004s
arran4 commented 2 years ago

Woot.