Add a temporary drop-in replacement for errors.Join #46188
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LOL |
|
For the record, here's what I get out of the API: |
|
One thing I'm wondering now, looking at the output; it looks like we have quite some indentation; Perhaps we should remove 1 level; And we should omit the Some variations: A. Just remove the top-level counter:question_mark: Does it feel weird if there's nothing at the first line of the error that summarizes "whats' wrong"? B. Remove the counter and use the first "wrapper" error as summaryIf there's only one error, we can use that error as the summary; C. Do we need the counter... at all?However, now the question is: does the
If we leave out the secondary counts, at least the top level counter is somewhat less ambiguous; And ... a that point, maybe (?) the whole count is not needed; |
(Off-topic) should we always have a summary on the first line?So... not sure what's best for this. As mentioned earlier it may feel a bit odd for the first line to only show In the examples on this PR, errors are due to networking-config validation, so we could add a summary; Looking what it would look like if there's (validation) errors in other areas, e.g.; However... now we're back at square 1: the first line doesn't have a summary. We can still add a "summary" at the top, but maybe that's just ... noise; |
neersighted
reviewed
Aug 11, 2023
Going back to this PR, and even before I reached this line I also thought the count doesn't add any value. I'll remove it. Except this, all other formatting suggestions aren't tied to this specific PR, but to #46183. So let's discuss that there. |
0b5b756 to
d2c4700
Compare
akerouanton
commented
Aug 16, 2023
| ) | ||
|
|
||
| func TestErrorJoin(t *testing.T) { | ||
| err := Join(errors.New("foobar"), fmt.Errorf("invalid config:\n%w", Join(errors.New("foo"), errors.New("bar")))) |
There was a problem hiding this comment.
If we use this instead:
err := Join(errors.New("foobar"), Join(errors.New("foo"), errors.New("bar")))
The output will be clumsy:
* foobar
* * foo
* bar
This is not great, but I believe it's better to defer to Join callers to do the right thing instead (eg. add an error count, a nice preamble message, etc...).
There was a problem hiding this comment.
The newline is a bit awkward indeed. Just gave it a quick try to see if we can make that automatic, but... it's tricky.
d2c4700 to
fe0e08e
Compare
thaJeztah
reviewed
Aug 16, 2023
There was a problem hiding this comment.
Left some suggestions; also pushed a quick branch if any of those make sense to you; akerouanton#4
As we have a hard time figuring out what moby#46099 should look like, this drop-in replacement will solve the initial formatting problem we have. It's made internal such that we can remove it whenever we want and unlike moby#46099 doesn't require thoughtful API changes. Signed-off-by: Albin Kerouanton <albinker@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
fe0e08e to
64de635
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
As we have a hard time figuring out what #46099 should look like, this drop-in replacement will solve the initial formatting problem we have. It's made internal such that we can remove it whenever we want and unlike #46099 doesn't require thoughtful API changes.
- How I did it
This new
joinErrorhas the exact same semantic has the one from stdlib. Only difference is how itsError()method format its message.- A picture of a cute animal (not mandatory but encouraged)