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

cli: deprecate cli.StatusError direct field usage #5738

Closed
wants to merge 1 commit into from

Conversation

Benehiko
Copy link
Member

@Benehiko Benehiko commented Jan 13, 2025

The exported cli.StatusError type may be used
by some directly within their own projects, making it difficult to update the struct's fields.

This patch converts the exported cli.StatusError to an interface instead, so that code wrapping the CLI would still be able to match the error and get the status code without exposing the fields.

This is a breaking change for those relying on creating a cli.StatusError{} and accessing the error's fields. For those using errors.As(err, &statusError) and err.(cli.StatusError) will be able to continue using it without breakage.

Users accessing the fields of cli.StatusError{}.StatusCode would need to use the new GetStatusCode() method.

- What I did

- How I did it

- How to verify it

- Description for the changelog

External code importing and using `cli.StatusError` directly (e.g. creating an instance of it) is deprecated.

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 40.74074% with 32 lines in your changes missing coverage. Please review.

Project coverage is 59.53%. Comparing base (b462778) to head (5063fa3).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5738      +/-   ##
==========================================
+ Coverage   59.51%   59.53%   +0.01%     
==========================================
  Files         346      346              
  Lines       29376    29378       +2     
==========================================
+ Hits        17483    17490       +7     
+ Misses      10923    10918       -5     
  Partials      970      970              

The exported `cli.StatusError` type may be used
by some directly within their own projects, making
it difficult to update the struct's fields.

This patch converts the exported `cli.StatusError` to
an interface instead, so that code wrapping the CLI
would still be able to match the error and get the
status code without exposing the fields.

This is a breaking change for those relying on creating
a `cli.StatusError{}` and accessing the error's fields.
For those using `errors.As(err, &statusError)` and
`err.(cli.StatusError)` will be able to continue using
it without breakage.

Users accessing the fields of `cli.StatusError{}.StatusCode`
would need to use the new `GetStatusCode()` method.

Signed-off-by: Alano Terblanche <[email protected]>
@Benehiko Benehiko force-pushed the deprecate-status-error branch from e6f65c4 to 5063fa3 Compare January 13, 2025 15:31
@laurazard
Copy link
Collaborator

laurazard commented Jan 14, 2025

The exported cli.StatusError type may be used by some directly within their own projects, making it difficult to update the struct's fields.

IMO turning this into an interface is fine (and I'm happy it matches what we do elsewhere – see errdefs) – but in general this is just the nature of working on large projects with many consumers.

However, turning this into an interface is still a breaking change, so it can't go out in a minor release. If we're breaking it anyway, it might be worth to go with a name we like/that matches the others, such as StatusErr or the like.

This is still something we can do in the meantime as I mentioned in the other PR.

@laurazard laurazard requested a review from thaJeztah January 14, 2025 15:07
@Benehiko
Copy link
Member Author

Yeah, this PR is not meant to go into a minor release and can be merged whenever. I made this PR to do a singular change to cli.StatusError that would at least keep compatibility (somewhat) to users using the type through errors.As(err, &statusErr).

This PR is essentially more so to force users to not use the cli.StatusError to instantiate their own errors (which doesn't make sense in any case).

@laurazard
Copy link
Collaborator

Yeah, this PR is not meant to go into a minor release and can be merged whenever.

I'm not sure whether this means it can be merged whenever or not but my point is that the same conditions under which this PR can be merged would allow for more substantial changes, so I'm not sure I understand the reasoning for a half-measure.

@Benehiko
Copy link
Member Author

Closing this PR and moving the commit back to the original PR since most of the discussion happened there #5666

@Benehiko Benehiko closed this Jan 20, 2025
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.

3 participants