rados: Implement Unwrap for OperationError#1222
Conversation
That's fine by me.
Preview is mainly there for public APIs to appear in a released version of the code but gives us an opportunity to change things if something gets noticed and needs updating during that preview period. In that case we're able to make a direct change instead of trying to deprecate and remove something people might have put into production. It's a silly example but what if someone complained that it should be Regardless, I'm not unwilling to make an exception for this but I think I'd want the other maintainers to chime in on that. ^ @anoopcs9 @ansiwen ** Since I just learned Go supports this. :-) |
anoopcs9
left a comment
There was a problem hiding this comment.
OperationError.Unwrap is technically a new API, but I don't believe it should be tagged as preview. Implementing this API for the OperationError type is more like a bug fix than a new feature.
It's a silly example but what if someone complained that it should be
Unwrap() errorvs.Unwrap() []error** we'd have an opportunity to have a discussion around that.
I wouldn't mind much. After all we are just adding to what's already returned as an error. I hope there isn't a case where there are StepErrors but OpError is empty.
** Since I just learned Go supports this. :-)
You are not alone 😀.
Thanks for the explanation of your process! In this case I'm not defining a new API, but essentially implementing an optional method of an already defined interface from the standard library, so there's not much to debate about how anyone wants it to look like. However, if the maintainers would prefer, I'm open to moving it out to a preview file of course.
I took care of omitting |
|
I'll officially +1 an exception to add this function without being a preview API first. @anoopcs9 it sounds like you are ok with it too, can you confirm? @ansiwen is not as active on this project lately and so I suggest waiting until Fri if he's got a reason to NACK it. But if we don't hear back by then and Anoop and I are OK with it we can move forward with that. (And even after the PR is merged we have wiggle room to adjust plans up until the next release day.) |
|
@Mergifyio rebase |
❌ Pull request can't be updated with latest base branch changesDetailsThis pull request seems to come from a fork, and Mergify needs the author's permission to update its branch. |
@dszoboszlay Can you please rebase the change? |
Exposing the operation and step errors wrapped by the OperationError makes it possible to use for example errors.Is to check the underlying error, and handle it properly. Signed-off-by: Dániel Szoboszlay <dszoboszlay@evroc.com>
5292aeb to
f9abe76
Compare
Pull request has been modified.
Done! (Sorry, I had the Allow edits by maintainers check box ticked, don't know why wasn't that enough.) |
|
It's mergify - they changed their configs (again) and I don't feel they do a good job of documenting the changes and it's hard to keep up. |
Merge Queue Status✅ The pull request has been merged at f9abe76 This pull request spent 7 seconds in the queue, with no time running CI. Required conditions to merge
|
Exposing the operation and step errors wrapped by the OperationError makes it possible to use for example errors.Is to check the underlying error, and handle it properly.
Checklist
UnwrapbecauseErrorisn't documented either. These are quite well-known methods for Go errors.//go:build ceph_preview→OperationError.Unwrapis technically a new API, but I don't believe it should be tagged as preview. Implementing this API for theOperationErrortype is more like a bug fix than a new feature.make api-updateto record new APIs