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

clienterrors: rename WorkerClientError to clienterrors.New #4278

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Jul 30, 2024

The usual convention to create new object is to prefix New* so this commit renames the WorkerClientError. Initially I thought it would be NewWorkerClientError() but looking at the package prefix it seems unneeded, i.e. clienterrors.New() already provides enough context it seems and it's the only error we construct.

We could consider renaming it to clienterror (singular) too but that could be a followup.

I would also like to make clienterror.Error implement the error interface but that should be a followup to make this (mechanical) rename trivial to review.

[this was https://github.com//pull/4143 but for some reason I couldn't reopen it]

The usual convention to create new object is to prefix `New*` so
this commit renames the `WorkerClientError`. Initially I thought
it would be `NewWorkerClientError()` but looking at the package
prefix it seems unneeded, i.e. `clienterrors.New()` already
provides enough context it seems and it's the only error we
construct.

We could consider renaming it to `clienterror` (singular) too
but that could be a followup.

I would also like to make `clienterror.Error` implement the
`error` interface but that should be a followup to make this
(mechanical) rename trivial to review.
@mvo5 mvo5 requested a review from croissanne July 30, 2024 07:04
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

lgtm! thank you :)

@mvo5 mvo5 merged commit 573b349 into osbuild:main Jul 31, 2024
43 checks passed
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.

2 participants