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

Decorate misbehave if err==nil #40

Open
g7r opened this issue Jan 19, 2022 · 5 comments
Open

Decorate misbehave if err==nil #40

g7r opened this issue Jan 19, 2022 · 5 comments

Comments

@g7r
Copy link
Contributor

g7r commented Jan 19, 2022

What happens? errorx.Decorate returns an error with the following properties:

  • e.IsOfType(...) always returns false
  • e.Type() returns synthetic.foreign

I suggest that errorx.Decorate(nil, "") should either return nil or panic.

@PeterIvanov
Copy link
Collaborator

PeterIvanov commented Jan 19, 2022

Come to think of it, I agree. This is not an intended way to use the library.
The issue is: this will potentially break some existing user code, which is a bad thing. At run time, which is worse.
I'm not sure at this point that the reasoning along the lines of "but there's nothing in godoc that promised to keep this behaviour" is good enough.
Need to think more about this.

@g7r
Copy link
Contributor Author

g7r commented Jan 19, 2022

Formally it looks more like not a breaking change but an API clarification. But I do agree that it might break some code.

I would certainly object against solutions like "let's just introduce DecorateV2" 😄. Personally I think that it isn't a big deal in changing something that wasn't specified as a public contract.

@PeterIvanov
Copy link
Collaborator

No, there may be only one Decorate.
I think a breaking change would require a major release, and we may have other things of that sort to bundle together - say, a neglected issue of Wrap naming.

@Tochemey
Copy link

Tochemey commented Sep 5, 2023

@PeterIvanov is this issue resolved?

@PeterIvanov
Copy link
Collaborator

@Tochemey I'm afraid it is not. Do you have a comment on the subject matter, one way or another?

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

No branches or pull requests

3 participants