User facing errors for State Checkout #2776
Conversation
Naatan
commented
Sep 28, 2023
•
Naatan
commented
 DX-2008 User facing errors for state checkout
|
# Conflicts: # internal/locale/locales/en-us.yaml # test/integration/checkout_int_test.go
github-actions
bot
changed the base branch from
master
to
version/0-42-0-RC1
| @@ -27,19 +28,24 @@ func (e *userFacingError) ErrorTips() []string { | |||
| return e.tips | |||
| } | |||
|
|
|||
| func (e *userFacingError) InputError() bool { | |||
There was a problem hiding this comment.
If I recall correctly from my previous PR we wanted to avoid user facing errors from colliding with existing error implementations. This is an implementation of the ErrorInput interface from the locale package.
There was a problem hiding this comment.
This isn't a collision. The problem I ran into is wanting to communicate an input failure on the user facing error side. So when I asserted the type I could set the user facing error, but also assert that it is due to user input.
If we use WrapInputError or something like that we add even more errors to the stack, and the API would be pretty awkward. This solves both those problems, you can now set a user facing error as an input error with NewUserFacing(msg, SetInput(true))
| other: The requested project [NOTICE]{{.V0}}[/RESET] does not exist under [NOTICE]{{.V1}}[/RESET]. Please ensure the owner is correct and that the project has been created. | ||
| err_api_project_not_found_unauthenticated: | ||
| other: The requested project [NOTICE]{{.V0}}/{{.V1}}[/RESET] could not be found | ||
| other: The requested project [NOTICE]{{.V1}}[/RESET] does not exist under [NOTICE]{{.V0}}[/RESET]. Please ensure the owner is correct and that the project has been created. |
There was a problem hiding this comment.
nit: Why are the variables here out of order?
There was a problem hiding this comment.
To keep the order consistent in the code itself, see lines 43 and 46 here:
cli/pkg/platform/model/projects.go
Lines 43 to 46 in d657e19
| branches, err := model.BranchNamesForProjectFiltered(proj.Owner(), proj.Name(), proj.BranchName()) | ||
| if err == nil && len(branches) > 1 { | ||
| // Suggest alternate branches | ||
| *rerr = errs.NewUserFacingError(locale.Tr( | ||
| "err_alternate_branches", | ||
| errNoMatchingPlatform.HostPlatform, errNoMatchingPlatform.HostArch, |
There was a problem hiding this comment.
nit: While I see the value in adding this suggestion it also adds complexity where we could just provide a more generic, but still helpful, message.
There was a problem hiding this comment.
This PR is just hoisting the existing error up to a user facing level. It's not seeking to make changes to the actual error.
| type ErrNoProject struct { | ||
| Inner | ||
| } |
There was a problem hiding this comment.
Having centralized errors in a package called rationalize is a bit confusing. Is this because the error is used by the rationalized functions?
There was a problem hiding this comment.
Rationalize is the term I want to use for this, so when you read "Rationalize" your mind should immediately make the connection.
"Centralized errors" isn't a term we ever used for this, it's just a generic way of thinking about this problem space. I want to be explicit about it.
Btw happy to explore other terms if you don't like "Rationalize". The intend is that it's both sensible and explicit.
There was a problem hiding this comment.
I'd prefer not to get caught up in the naming. This makes enough sense to me.
| type ErrNoProject struct { | ||
| Inner | ||
| } |
There was a problem hiding this comment.
I'd prefer not to get caught up in the naming. This makes enough sense to me.
