-
Notifications
You must be signed in to change notification settings - Fork 38
Add New...f constructors for all service errors #221
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| package serviceerror | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
@@ -13,13 +15,20 @@ type ( | |
| } | ||
| ) | ||
|
|
||
| // NewAlreadyExist returns new AlreadyExists error. | ||
| func NewAlreadyExist(message string) error { | ||
| // NewAlreadyExists returns new AlreadyExists error. | ||
| func NewAlreadyExists(message string) error { | ||
| return &AlreadyExists{ | ||
| Message: message, | ||
| } | ||
| } | ||
|
|
||
| // NewAlreadyExistsf returns new AlreadyExists error with formatted message. | ||
| func NewAlreadyExistsf(format string, args ...any) error { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users are not really expected to create these errors, do we really need new user-facing helpers? What is the use case driving this PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These errors are constructed mostly on server. User is a server dev. We have hundreds of usages with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User of this library is more than server dev. I don't think we need to update this user-facing package with server-dev-only utilities. You aren't getting rid of the helpers, you're moving them to the this public, user-facing library for everyone. If the user is server dev, can it be put somewhere only for server dev?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Go SDK has several instances of Also, it would be pretty weird to have the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But it's used in the SDK and by users, what repo should it be in? Also, you have the ability to add these helpers in whatever repo you think, there's no requirement to add them in this one, so you don't have to perpetuate your mistake.
Right, I am not questioning the validity of the methods, I am questioning the user-facing aspect. These types of things are added in the Go SDK because we expect users to use them and therefore we are ok with the increased API surface area, stability guarantees, etc.
I assume you mean weird to server developers, because it is not weird to everyone else that uses this library to not have those server-only helpers. We have to always keep users in mind, not just ourselves. It is very important. My primary concern is adding server-only helpers to this repository. It is less of a concern with these of course since they are simple constructor overloads. But in general it's a bad pattern to add "server-developer-only helpers" to this user-facing library. If y'all feel strongly enough about giving all users these helpers, ok (let me know via response), but we would prefer server-only code to be server-only code as a general rule (of course just some overloaded constructors is not that big of a deal).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are not server-only. And yes, they should be here with the non-f constructors.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is at odds with some of the other comments, e.g. "It is not used by SDK. Only by server", hence the concern/thread here. Marking approved though I do think it's important to keep in mind that we should not clutter user packages with server-only needs if we can help it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not used by SDK yet 🙂 |
||
| return &AlreadyExists{ | ||
| Message: fmt.Sprintf(format, args...), | ||
| } | ||
| } | ||
|
|
||
| // Error returns string message. | ||
| func (e *AlreadyExists) Error() string { | ||
| return e.Message | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't make backwards-incompatible changes to this package IMO (even if users shouldn't really be using it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not used by SDK. Only by server. But I agree... I should leave old constructor as deprecated.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not know everyone that uses this. The
go.temporal.io/apilibrary is one of the most included depended-upon libraries by Temporal users (granted usually transitively). Many users have proxies, client mocks, etc that may take advantage of these things. We should not adjust this library as if it's only used by server, https://github.com/temporalio/temporal is a better place for code that is only used by server.