-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Error handling #73
base: master
Are you sure you want to change the base?
Error handling #73
Changes from 2 commits
08adbda
14095cc
67727dd
b51e64d
6d4f14f
a5a215d
4b2d6a1
23c4714
b69874f
7d97e96
ce8cff1
0abebc2
acf49e9
a0f11e3
70f69c8
1dfe73f
7678c37
e69ce9c
aef0a29
8525f6b
1a61754
a55aac2
5f3bef9
55df13e
9e30bdf
1e96c15
cf8d1b3
804f4d3
681cc6e
3631125
a8f0586
46e2a5c
51fa6f1
3ffa099
0f8a325
647cf9e
542d6e5
7bd8708
bb39d64
0fcebed
f5b4408
93ca35f
84a339a
02c6335
9fab3f0
1e5b6e4
27bb2dd
909bd3b
3e4aba3
f044c04
008589d
baa1c1d
ea37e62
8d1cec9
68b453c
b866620
2f726f2
9a99514
20061ba
6591742
c331f7a
ea4ce01
f76e1f3
e616cd6
2520cb0
4c3688b
1d9da2e
3a306dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package ics | ||
|
||
const ( | ||
MalformedCalendarExpectedVCalendarError = "malformed calendar; expected a vcalendar" | ||
MalformedCalendarExpectedBeginError = "malformed calendar; expected begin" | ||
MalformedCalendarExpectedEndError = "malformed calendar; expected a end" | ||
MalformedCalendarExpectedBeginOrEndError = "malformed calendar; expected begin or end" | ||
|
||
MalformedCalendarUnexpectedEndError = "malformed calendar; unexpected end" | ||
MalformedCalendarBadStateError = "malformed calendar; bad state" | ||
MalformedCalendarVCalendarNotWhereExpectedError = "malformed calendar; vcalendar not where expected" | ||
|
||
StartOrEndNotYetDefinedError = "start or end not yet defined" | ||
PropertyNotFoundError = "property not found" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order to be reliably compared these should be errors rather than strings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (But technically not a blocker.) |
||
ExpectedOneTZIDError = "expected one TZID" | ||
|
||
TimeValueNotMatchedError = "time value not matched" | ||
TimeValueMatchedButUnsupportedAllDayTimeStampError = "time value matched but unsupported all-day timestamp" | ||
TimeValueMatchedButNotSupportedError = "time value matched but not supported" | ||
|
||
ParsingComponentPropertyError = "parsing component property" | ||
ParsingComponentLineError = "parsing component line" | ||
ParsingLineError = "parsing line" | ||
ParsingCalendarLineError = "parsing calendar line" | ||
ParsingPropertyError = "parsing property" | ||
ParseError = "parse error" | ||
|
||
MissingPropertyValueError = "missing property value" | ||
|
||
UnexpectedASCIICharError = "unexpected char ascii" | ||
UnexpectedDoubleQuoteInPropertyParamValueError = "unexpected double quote in property param value" | ||
|
||
UnbalancedEndError = "unbalanced end" | ||
OutOfLinesError = "ran out of lines" | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package ics | |
|
||
import ( | ||
"bytes" | ||
"errors" | ||
"fmt" | ||
"io" | ||
"log" | ||
|
@@ -174,7 +175,7 @@ func ParseProperty(contentLine ContentLine) (*BaseProperty, error) { | |
t := r.IANAToken | ||
r, np, err = parsePropertyParam(r, string(contentLine), p+1) | ||
if err != nil { | ||
return nil, fmt.Errorf("parsing property %s: %w", t, err) | ||
return nil, fmt.Errorf("%s %s: %w", ParsingPropertyError, t, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We won't be able to catch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this technical is a bug |
||
} | ||
if r == nil { | ||
return nil, nil | ||
|
@@ -198,7 +199,7 @@ func parsePropertyParam(r *BaseProperty, contentLine string, p int) (*BaseProper | |
case '=': | ||
p += 1 | ||
default: | ||
return nil, p, fmt.Errorf("missing property value for %s in %s", k, r.IANAToken) | ||
return nil, p, fmt.Errorf("%s for %s in %s", MissingPropertyValueError, k, r.IANAToken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the other one. Multiple I guess that means we should push the go.mod up to that. -- Based on this I'm in support. 1.20 is less than 2 years old. But most stacks are rather current. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah this one is just missing a |
||
} | ||
for { | ||
if p >= len(contentLine) { | ||
|
@@ -207,7 +208,7 @@ func parsePropertyParam(r *BaseProperty, contentLine string, p int) (*BaseProper | |
var err error | ||
v, p, err = parsePropertyParamValue(contentLine, p) | ||
if err != nil { | ||
return nil, 0, fmt.Errorf("parse error: %w %s in %s", err, k, r.IANAToken) | ||
return nil, 0, fmt.Errorf("%s: %w %s in %s", ParseError, err, k, r.IANAToken) | ||
} | ||
r.ICalParameters[k] = append(r.ICalParameters[k], v) | ||
switch rune(contentLine[p]) { | ||
|
@@ -253,10 +254,10 @@ func parsePropertyParamValue(s string, p int) (string, int, error) { | |
for ; p < len(s) && !done; p++ { | ||
switch s[p] { | ||
case 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08: | ||
return "", 0, fmt.Errorf("unexpected char ascii:%d in property param value", s[p]) | ||
return "", 0, fmt.Errorf("%s:%d in property param value", UnexpectedASCIICharError, s[p]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. %w |
||
case 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1A, 0x1B, | ||
0x1C, 0x1D, 0x1E, 0x1F: | ||
return "", 0, fmt.Errorf("unexpected char ascii:%d in property param value", s[p]) | ||
return "", 0, fmt.Errorf("%s:%d in property param value", UnexpectedASCIICharError, s[p]) | ||
case '\\': | ||
r = append(r, []byte(FromText(string(s[p+1:p+2])))...) | ||
p++ | ||
|
@@ -276,7 +277,7 @@ func parsePropertyParamValue(s string, p int) (string, int, error) { | |
done = true | ||
continue | ||
} | ||
return "", 0, fmt.Errorf("unexpected double quote in property param value") | ||
return "", 0, errors.New(UnexpectedDoubleQuoteInPropertyParamValueError) | ||
} | ||
r = append(r, s[p]) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case and others I think there there are too many errors. I would prefer to see something like:
Then it be embellished with detail when it is returned like @arran4 suggested in #98:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not bothered by the quantity, but I do think regardless, it should be wrapped in a
ErrorMalformedCalendar
that is a good suggestion either direction.I have some things to do I will have to get back to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also not a blocker.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quantity has practical concerns for me: Say I'm writing a server and choosing a HTTP status code. If the user uploaded an invalid calendar I want to return 400. If something else happened, such as the connection dropped and the io.Reader returned an EOF, then I want to return 500. I now have to check if the error was any one of the many parse errors and hope that this module doesn't make any new ones. I'm happy to drop this thread but want to point out that once you ship all theses errors, you can't take it back without a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is possible using
error.Is
sorry I didn't ignore this concern I just failed to follow up on it. I am happy to work through a simple example if you like?