Error handling #73

ManoloTonto1 · 2023-07-12T17:28:24Z

This pr contains all errors in a file called errors.go for better error handling for the end-user.
closes #71
all tests a passing.

- Add methods for VTodo and VJournal - Changed the shared properties to be attached to the ComponentBase so those methods don't need to be repeated across all attributes that share them - If a property is shared across some, but not all components, then it's a private method on the ComponentBase and public method on the Components (eg LOCATION for Event and Todo but not Journal) - Shifted position of some Event methods in the file to live after the VEvent struct

- Add methods for Alarm, Timezone, and FreeBusy - Only methods for creating and adding right now, will add methods for the properties unique to them later

- Recursive call for Alarms - SetGeo was not a public method - Missing comma in struct creation

…ndling

arran4 · 2023-07-13T07:24:13Z

Thanks @ManoloTonto1 I think I might make some edits to this. (When I get a bit more time.)

I'm thinking along the lines of:

Prefixing with Err instead of suffixing with Error - Why? For intelisense users like myself.
Moving the error.New to the consts - Why? So that errors.Is() is usable and string operations aren't required
Increase the use of fmt.Errorf() - Why? To allow for the %w operator for error chaining.
In the case where fmt.Errorf() is used, combine with a const ErrX = error.New() as errors.*() support multiple child errors now, and for consistency

I do like the initiative, and that you have put it in one file, and made it more consistent. You have also kept it lower case and removed the word error from the error string which is some form of error or warning somewhere. Thanks.

ManoloTonto1 · 2023-07-13T10:59:34Z

Great ideas. If I get a chance this week I can edit my pr and do some of the things you pointed out. 💯

arran4

As pre previous comment

arran4 · 2023-07-25T04:17:58Z

@ManoloTonto1 I found some time to make some changes, can you let me know what you think. I have put them as a PR into your PR: (well to your master branch:) ManoloTonto1#1

arran4 · 2023-07-25T04:18:33Z

Moving the error.New to the consts - Why? So that errors.Is() is usable and string operations aren't required

Also turns out there was an error in this one. The consts needed to be converted into a var

arran4 · 2023-07-25T04:23:01Z

Seems my change pushes the requirement up to go 1.20.

I will have to tag the current version before merging.

ManoloTonto1 · 2023-07-25T07:34:23Z

@ManoloTonto1 I found some time to make some changes, can you let me know what you think. I have put them as a PR into your PR: (well to your master branch:) ManoloTonto1#1

Ok goos I will give it a look when I get some time, i am currently on a holiday with my family. As soon as I get the chance to review i will let you know. 😋

Bumps gopkg.in/yaml.v3 from 3.0.0-20210107192922-496545a6307b to 3.0.0. --- updated-dependencies: - dependency-name: gopkg.in/yaml.v3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

…in/yaml.v3-3.0.0 Bump gopkg.in/yaml.v3 from 3.0.0-20210107192922-496545a6307b to 3.0.0

ManoloTonto1 · 2023-09-12T21:29:48Z

@arran4 Hey, I just got some time to work on this in the weekend. I will get back to you after I get these issues resolved.

arran4 · 2023-09-13T00:36:45Z

Sure, let me know if you have any questions or issues.

Add VEvent.GetDtStampTime

add possibility to remove VEvent from calendar by id

Add ComponentPropertyPriority; the VTODO component can have that

ManoloTonto1 · 2024-09-25T09:10:59Z

hey @arran4 I fixed the merge conflicts, is there anything else that should be done before this is merged?

brackendawson · 2024-09-25T12:28:47Z

errors.go

+	MalformedCalendarVCalendarNotWhereExpectedError = "malformed calendar; vcalendar not where expected"
+
+	StartOrEndNotYetDefinedError = "start or end not yet defined"
+	PropertyNotFoundError        = "property not found"


In order to be reliably compared these should be errors rather than strings.

(But technically not a blocker.)

brackendawson · 2024-09-25T12:32:20Z

errors.go

+	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"


In this case and others I think there there are too many errors. I would prefer to see something like:

ErrorMalformedCalendar = errors.New("malformed calendar")

Then it be embellished with detail when it is returned like @arran4 suggested in #98:

return nil, fmt.Errorf("%w: expected begin or end", ErrorMalformedCalendar)

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.

(Also not a blocker.)

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.

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?

arran4 · 2024-09-26T02:04:23Z

Running tests and lint.

arran4 · 2024-09-26T02:25:54Z

Lint still fails, I think we had to bump up the go version, that seems acceptable now given how old the version we are using is. I would like to hear from people using a go version older than something released in the last 4-5 years.

arran4 · 2024-09-26T04:24:11Z

.github/workflows/test.yml

    strategy:
      matrix:
-        go-version: [1.14.x, 1.15.x, 1.16.x, 1.17.x]
-        os: [ubuntu-latest, macos-latest, windows-latest]
+        go-version: ['1.14.15', '1.15.15', '1.16.15', '1.17.13', '1.22.3']


Any reason for the preference for concrete versions here?

arran4 · 2024-09-26T04:27:22Z

.gitignore

@@ -1 +1,2 @@
 /.idea/
+/testdata/serialization/actual


Not necessary?

arran4

LGTM it's there are a couple minor issues with error wrapping. - I suggest they get fixed but I can accept that in another PR to get this one across the board.

No major blockers. (Barring lint issues.)

Let me know what you want to do.

arran4 · 2024-09-26T04:29:35Z

property.go

@@ -174,7 +234,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)


We won't be able to catch ParsingPropertyError error.Is and error.As now support multiple errors, so we can use %w twice, past a certain go version... Since 1.20: https://www.reddit.com/r/golang/comments/z870te/multiple_error_wrapping_is_coming_in_go_120/

I guess this technical is a bug

arran4 · 2024-09-26T04:31:01Z

property.go

 	switch rune(contentLine[p]) {
 	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)


As per the other one. Multiple %w are supported as of 1.20..

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.

Ah this one is just missing a %w wish github had the same feature gitlab does "suggest a fix"

arran4 · 2024-09-26T04:31:20Z

property.go

@@ -253,11 +319,14 @@ 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])


The merge-base changed after approval.

arran4

As before -- The approval expired for some reason.

arran4 · 2024-09-26T04:44:55Z

property.go

 	switch rune(contentLine[p]) {
 	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)


Ah this one is just missing a %w wish github had the same feature gitlab does "suggest a fix"

The merge-base changed after approval.

ManoloTonto1 · 2024-09-26T08:35:08Z

LGTM it's there are a couple minor issues with error wrapping. - I suggest they get fixed but I can accept that in another PR to get this one across the board.

No major blockers. (Barring lint issues.)

Let me know what you want to do.

Let me fix the lint issues, then we can merge. Maybe we can tackle the other issue some other time.

arran4 · 2024-09-26T08:39:46Z

OK

arran4 · 2024-09-27T03:48:38Z

Conflicting files

components.go
errors.go

Which is:

  calendar_test.go:5:2: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
  	"io/ioutil"

arran4 · 2024-09-29T06:37:55Z

@ManoloTonto1 I have fixed the conflicts in the PR into your PR: ManoloTonto1#1

Once you merge that in this should work then I can.... review it again. I fixed the issues without looking at anything else sorry.

It's also possible that the PR I created earlier doesn't actually resolve some of your issues. However I would encourage you to merge it into your PR. I can also create a new PR here and then merge it myself, I will retain your commits if I do.

* Add leaks and vunerability checks * Requires secrets now * Renamed interface as it's useful own it's own. Made it public. Added a comment * Some suggestions, this could be improved still. * Version bump required by this change. * 19 too fails. * Should be able to distinguish unset from invalid time properties * Improve error for property not found Co-authored-by: Arran Ubels <[email protected]> * Remove deprecated ioutil * Move targeted Go version to 1.20 And test the target forever * Add method to remove property Fixes arran4#95 * Merged * New tool * Test fixed. * Fixed test failure. (Error wrapping needed to be considered in EOF checks.) * %s => %w fixes and an additional error. --------- Co-authored-by: Bracken Dawson <[email protected]> Co-authored-by: Daniel Lublin <[email protected]>

arran4 · 2024-10-06T01:14:10Z

Cool. Now to sort though the new conflicts.

arran4 · 2024-10-15T07:43:36Z

Tried to resolve conflicts in: ManoloTonto1#2

If preferable I can run with this entirely in: #109 your commits will still show up in the code base.

I'm going to give this PR until the end of the month then make a decision on my own.

* Add leaks and vunerability checks * Requires secrets now * Renamed interface as it's useful own it's own. Made it public. Added a comment * fix: omit zone in "AllDay" event helpers For a date-only event (i.e., an event that lasts for the full day) the iCalendar specification indicates that the value for DTSTART / DTEND should be a DATE https://icalendar.org/iCalendar-RFC-5545/3-6-1-event-component.html > The "VEVENT" is also the calendar component used to specify an > anniversary or daily reminder within a calendar. These events have a > DATE value type for the "DTSTART" property instead of the default value > type of DATE-TIME. If such a "VEVENT" has a "DTEND" property, it MUST be > specified as a DATE value also The DATE format (https://icalendar.org/iCalendar-RFC-5545/3-3-4-date.html) should omit both time and zone/location elements and additionally notes that "The "TZID" property parameter MUST NOT be applied to DATE properties" As per the specification, this PR also adds an explicit "VALUE=DATE" parameter when the AllDay helpers were called, to indicate that the property's default value type has been overridden and the VEVENT is intended to be an all-day event https://icalendar.org/iCalendar-RFC-5545/3-2-20-value-data-types.html Finally the SetDuration call has been updated to preserve the "AllDay" characteristics if the existing start or end has been specified in DATE format, which is also a requirement of the spec. Contributes-to: arran4#55 * calendar parsing url support * usage example, README * added functionnal option pattern for url parsing * Should be able to distinguish unset from invalid time properties * Improve error for property not found Co-authored-by: Arran Ubels <[email protected]> * Remove deprecated ioutil * Move targeted Go version to 1.20 And test the target forever * Add method to remove property Fixes arran4#95 * Merged * New tool * Reintegration of arran4#67 * Resynced and moved some functions around arran4#78 * Test fixed. * Create bug.md * Create other.md * Create default.md * Rename default.md to pull_request_template.md * Minor update * refactor: remove unnecessary named return values, harmonizing code base * refactor: rename var to reflect what it is These functions take optional variadic PropertyParam arguments, in ical speak they are not properties, but parameters for a property. * refactor: use ReplaceAll * refactor: prefer switch for readability * refactor: use consistent receiver names * refactor: rename unused arg as '_' * Tests added. * Some larger upgrades. * Fix * Some multiple-ness. * Duplication issue fixed. * Merged * Merge remote-tracking branch 'origin/master' into issue97 * origin/master: Duplication issue fixed. Some multiple-ness. Test fixed. Resynced and moved some functions around arran4#78 added functionnal option pattern for url parsing usage example, README calendar parsing url support # Conflicts: # calendar.go # calendar_test.go # components.go --------- Co-authored-by: Dominic Evans <[email protected]> Co-authored-by: tradulescu <[email protected]> Co-authored-by: Bracken Dawson <[email protected]> Co-authored-by: Daniel Lublin <[email protected]>

arran4 · 2024-10-21T11:40:33Z

Turns out my PR wasn't enough, there are still more conflicts. -_-

ManoloTonto1 · 2024-10-28T15:56:42Z

Turns out my PR wasn't enough, there are still more conflicts. -_-

Let me resolve them now, I will get back to you

arran4 · 2024-10-29T03:05:36Z

Thanks. We can also switch to: #109 which I believe I have solved them. I suspect that the conflicts remaining are due to the way github tracks merge history -- I suspect it ignores merges.

spslater and others added 9 commits April 3, 2023 03:36

Add Alarm, Timezone, and FreeBusy Property Methods

14095cc

- Add methods for Alarm, Timezone, and FreeBusy - Only methods for creating and adding right now, will add methods for the properties unique to them later

Fix Typos in a couple places

67727dd

- Recursive call for Alarms - SetGeo was not a public method - Missing comma in struct creation

Merge branch 'arran4:master' into add-todo-journal

b51e64d

Add Comment and Category Properties to Components

6d4f14f

Merge branch 'master' into add-todo-journal

a5a215d

Add changes for All Day Events to All Day Todos

4b2d6a1

added errors.go and converted errors to constants for better error ha…

23c4714

…ndling

fixed some namings

b69874f

arran4 self-requested a review July 25, 2023 00:59

arran4 assigned ManoloTonto1 Jul 25, 2023

arran4 requested changes Jul 25, 2023

View reviewed changes

dependabot bot and others added 2 commits September 6, 2023 23:40

Bump gopkg.in/yaml.v3 from 3.0.0-20210107192922-496545a6307b to 3.0.0

7d97e96

Bumps gopkg.in/yaml.v3 from 3.0.0-20210107192922-496545a6307b to 3.0.0. --- updated-dependencies: - dependency-name: gopkg.in/yaml.v3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

Merge pull request arran4#76 from arran4/dependabot/go_modules/gopkg.…

ce8cff1

…in/yaml.v3-3.0.0 Bump gopkg.in/yaml.v3 from 3.0.0-20210107192922-496545a6307b to 3.0.0

galenwarren and others added 8 commits November 8, 2023 10:15

Add VEvent.GetDtStampTime

0abebc2

Merge pull request arran4#80 from coachclientconnect/add-get-timestamp

acf49e9

Add VEvent.GetDtStampTime

add possibility to remove VEvent from calendar by id

a0f11e3

undo formatting change

70f69c8

remove redundant return statement

1dfe73f

Merge pull request arran4#81 from zachmann/master

7678c37

add possibility to remove VEvent from calendar by id

Add ComponentPropertyPriority; the VTODO component can have that

e69ce9c

Merge pull request arran4#82 from quite/add-component-priority

aef0a29

Add ComponentPropertyPriority; the VTODO component can have that

Merge branch 'master' of github.com:ManoloTonto1/golang-ical

2520cb0

fixed som functions that are using the wrong Struct Embeddings

4c3688b

brackendawson reviewed Sep 25, 2024

View reviewed changes

arran4 reviewed Sep 26, 2024

View reviewed changes

.gitignore

@@ -1 +1,2 @@

/.idea/

/testdata/serialization/actual

Copy link

Owner

arran4 Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary?

arran4 previously approved these changes Sep 26, 2024

View reviewed changes

This was referenced Sep 28, 2024

Add method to remove property #96

Merged

Some suggestions, this could be improved still. ManoloTonto1/golang-ical#1

Merged

This was referenced Oct 15, 2024

Issue 97: Support for Alternative Representations #106

Merged

Error handling conflicts again ManoloTonto1/golang-ical#2

Merged

arran4 mentioned this pull request Oct 16, 2024

Error handling conflicts again #109

Open

arran4 added bug Something isn't working large labels Oct 16, 2024

Error handling #73

Are you sure you want to change the base?

Error handling #73

Conversation

ManoloTonto1 commented Jul 12, 2023 • edited Loading

arran4 commented Jul 13, 2023 • edited Loading

ManoloTonto1 commented Jul 13, 2023

arran4 left a comment

Choose a reason for hiding this comment

arran4 commented Jul 25, 2023

arran4 commented Jul 25, 2023

arran4 commented Jul 25, 2023 • edited Loading

ManoloTonto1 commented Jul 25, 2023

ManoloTonto1 commented Sep 12, 2023

arran4 commented Sep 13, 2023

ManoloTonto1 commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arran4 commented Sep 26, 2024

arran4 commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arran4 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arran4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManoloTonto1 commented Sep 26, 2024

arran4 commented Sep 26, 2024

arran4 commented Sep 27, 2024 • edited Loading

arran4 commented Sep 29, 2024

arran4 commented Oct 6, 2024

arran4 commented Oct 15, 2024 • edited Loading

arran4 commented Oct 21, 2024 • edited Loading

ManoloTonto1 commented Oct 28, 2024

arran4 commented Oct 29, 2024

ManoloTonto1 commented Jul 12, 2023 •

edited

Loading

arran4 commented Jul 13, 2023 •

edited

Loading

arran4 commented Jul 25, 2023 •

edited

Loading

arran4 left a comment •

edited

Loading

arran4 commented Sep 27, 2024 •

edited

Loading

arran4 commented Oct 15, 2024 •

edited

Loading

arran4 commented Oct 21, 2024 •

edited

Loading