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

Add Go 1.20 errors.Join support #118

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

dhartunian
Copy link
Contributor

@dhartunian dhartunian commented Aug 17, 2023

This PR contains the Join implementation from #106 and adds
some adjustments to fit the rest of the library's style.

We move the newly added Join implementation that mirrors
the go 1.20 stdlib function into a separate package like the rest of
our custom error types.

Some simple unit tests are added and Join wrappers are also
integrated into the datadriven formatting test. Our existing format
code for multi-cause errors is compatible with this new type which
allows us to remove the custom formatter from the implementation.


Please review last 2 commits. First one is #115.


This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

May I suggest:

  • move the new join.go to its sub-package join immediately in the 1st commit.
  • ensure the joinError type also implements SafeFormatError. Then express Error/Format using that, using the model already set by other types.
  • in the 2nd commit, make the top-level join Join point to a wrapper in package errutil that follows the example of Wrap and also automatically adds a call to WithStack.

Reviewable status: 0 of 18 files reviewed, all discussions resolved

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Also, for your consideration:

What about not adding a joinError type altogether, and instead construct an opaqueMultiErrorLeaf with all the formatting done upfront (in errutil)?

(This would need to use a redact.RedactableString)

Reviewable status: 0 of 18 files reviewed, all discussions resolved

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

(maybe my last consideration is not so great - the opaque type's support for redactable strings is janky)

If you do not do that, consider adding a decoder for the new joinError type.

Reviewable status: 0 of 18 files reviewed, all discussions resolved

@dhartunian
Copy link
Contributor Author

@knz PTAL

  • added custom encoder/decoder for Join (based on API extended in latest update to add formatting for multi-cause errors #115)
  • merged the two commits into 1 to put the Join impl where it needs to be
  • added custom formatter
  • added public API wrapper with stack

@dhartunian dhartunian requested a review from knz August 22, 2023 12:20
@knz
Copy link
Contributor

knz commented Aug 22, 2023

CI is trying to tell you something.

I'd say let's merge #115 then you can rebase this which will make the review easier.

@dhartunian dhartunian force-pushed the go-1.20-join branch 2 times, most recently from 9b2dd64 to 5a7989c Compare August 22, 2023 13:03
@dhartunian
Copy link
Contributor Author

CI is trying to tell you something.

lol fixed 😄

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 18 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhartunian)


go.mod line 3 at r6 (raw file):

module github.com/cockroachdb/errors

go 1.18

I'm not sure you wanted to change this?


join/join.go line 76 at r6 (raw file):

func (e *joinError) SafeFormatError(p errbase.Printer) error {
	p.Print(e.Error())

Please do it the other way around: put the logic in here, then do Error() string { return redact.StringWithoutMarkers(e) }

Otherwise you'd be throwing redactability out of the window.

(you can see the effects of this in the format tests)

This commit introduces a drop-in replacement of the go errors lib
`Join` method which constructs a simple multi-cause error object.

Some simple unit tests are added and `Join` wrappers are also
integrated into the datadriven formatting test. Our existing format
code for multi-cause errors is compatible with this new type which
allows us to remove the custom formatter from the implementation.

The public Join API contains wrappers that automatically attach
stacktraces to the join errors.

Signed-off-by: Steve Coffman <steve@khanacademy.org>
Copy link
Contributor Author

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Reviewable status: 13 of 25 files reviewed, 2 unresolved discussions (waiting on @knz)


go.mod line 3 at r6 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I'm not sure you wanted to change this?

whoops. done.


join/join.go line 76 at r6 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Please do it the other way around: put the logic in here, then do Error() string { return redact.StringWithoutMarkers(e) }

Otherwise you'd be throwing redactability out of the window.

(you can see the effects of this in the format tests)

done.

@dhartunian dhartunian requested a review from knz August 23, 2023 12:35
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

💯

Reviewed 12 of 12 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhartunian)

@dhartunian dhartunian merged commit 2fb1006 into cockroachdb:go-1.20-upgrade Aug 23, 2023
2 checks passed
@dhartunian dhartunian deleted the go-1.20-join branch August 23, 2023 15:14
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.

3 participants