User-facing errors in example runner #2741

MDrakos · 2023-09-01T22:08:30Z

DX-2151 User facing errors for example runner

internal/errs/errs.go

internal/runners/hello/hello_example.go

Naatan · 2023-09-06T20:54:30Z

internal/runners/hello/hello_example.go

 func (h *Hello) Run(params *RunParams) error {
+	err := h.run(params)
+	if err != nil {
+		for _, unpackedError := range errs.Unpack(err) {
+			switch unpackedError.(type) {
+			case *runbits.NoNameProvidedError:
+				// Errors that we are looking for should be wrapped in a user-facing error.
+				// Ensure we wrap the top-level error returned from the runner and not
+				// the unpacked error that we are inspecting.
+				return &HelloUserFacingError{
+					locale.WrapInputError(
+						err,
+						"hello_err_no_name",
+						"Cannot say hello because no name was provided.",
+					),
+				}
+
+			default:
+				// If this is not a specific error we are looking for, do nothing.
+				continue
+			}
+		}
+
+		return locale.WrapError(err, "hello_cannot_say", "Cannot say hello.")
+	}
+
+	return nil
+}


I don't like the approach of having a Run calling another run, where the first one is basically a post-processor of the latter. That feels like an inverse of responsibilities in how you traverse the code when reading.

I think we should have a single Run function, and we defer the error "post processor". So at the start of the run function we'd have something like

func processError(err *error) { ... *err = errs.WrapUserFacing(err, "You broke it!") ... } func (h *Hello) Run(params *RunParams) (rerr error) { defer processError(&rerr) ... }

I prefer the approach that is currently in the PR. While having a function that does post-processing + defer fits nicely in a mental model in practice I find that when reading the code it requires more jumping around, or at least remembering that you have to jump to the defer function after reading the runner logic. This isn't a problem with the current code as you read the Run function, then the function that it calls, and return to the caller.

Post-processing with a defer also requires a named return value which is different from the vast majority of the functions in our codebase and also adds another layer of complexity when reading the code.

This is all subjective and in the end, it's your call.

The pattern of the named return is not uncommon. It is used frequently in our codebase.

I'm open to other solutions, but between the current implementation and what I am suggesting I'd prefer my suggestion. Giving each runner a redundant Run() function that you have to interpret to understand does not feel like the better solution to me, but you're right, that is subjective. But, given this is meant as a reference for our codebase rather than a one-off piece of code I'm gonna be nitpicky and insist on either my solution or an alternate yet to be determined one.

internal/runners/hello/hello_example.go

Naatan · 2023-09-06T21:05:13Z

pkg/cmdlets/errors/errors.go

@@ -93,8 +93,6 @@ func ParseUserFacing(err error) (int, error) {
 		return 0, nil
 	}

-	_, hasMarshaller := err.(output.Marshaller)


This change is breaking the original intend of the function. Just way predated the new design and isn't looking to address the same use-case. What you effectively found here is that the term "User Facing" was already in use, and we need to either resolve the conflict of overlapping terms, or ensure that they are compatible.

Looking through the code it seems hasMarshaller will mainly identify whether we already have an OutputError on our hands, so we don't redundantly wrap it. While I don't see any scenario's in which this could be true, do we want to risk breaking this behaviour for this PR?

Please take some time to understand what OutputError{} is doing and how the new concept of user facing errors plays into this.

From what I can grok the OutputError type adds extra post-processing and the handling in this function ensures that errors returned all have a similar UX. I've wrapped the user-facing error in this OutputError type as well to ensure it's consistent with all of the other errors we are presenting to users.

I've also added back the hasMarshaller check as you were right, we don't want to wrap the error again in the OutputError type.

Adjust how we handle errors in example runner Add back marshaller handling in user facing error parsing

internal/errs/errs.go

internal/runbits/hello_example.go

internal/runners/hello/hello_example.go

Naatan · 2023-09-08T17:12:56Z

pkg/cmdlets/errors/errors.go

+	uerr, ok := errs.IsUserFacing(err)
+	if ok {
+		logging.Debug("Returning user facing error, error stack: \n%s", errs.JoinMessage(err))
+		return code, &OutputError{uerr}


I don't think OutputError produces the error string from UserFacingError()?

Please make sure you run this code to ensure it produces the error message you expect.

It does here: https://github.com/ActiveState/cli/blob/master/pkg/cmdlets/errors/errors.go. I've confirmed this from running the code.

I don't understand, you linked to code on master which definitely would not be handling this new concept. Wrong link?

Whoops, yes wrong link.

I've run the code and confirmed the output from UserError() is there. I believe it's done via the output mediator here: https://github.com/ActiveState/cli/blob/DX-2151/internal/output/mediator.go#L54

Add back interface Remove unused error type

Naatan

Suggest we try to do some pair programming tomorrow to resolve this PR faster.

internal/errs/centralized.go

Naatan · 2023-09-12T21:50:57Z

internal/errs/errs.go

+}
+
+func (e *userFacingError) Error() string {
+	return JoinMessage(Wrap(e.wrapped, e.message))


Suggested change

return JoinMessage(Wrap(e.wrapped, e.message))

return "User Facing Error: " + e.UserError()

internal/errs/errs.go

internal/runners/hello/hello_example.go

internal/errs/errs.go

internal/runners/hello/hello_example.go

pkg/cmdlets/errors/errors.go

Update error tip handling

internal/errs/userfacing.go

pkg/cmdlets/errors/errors.go

mitchell-as · 2023-09-26T20:03:54Z

internal/runbits/hello_example.go

+type NoNameProvidedError struct {
+	*errs.WrapperError
+}
+
 func SayHello(out output.Outputer, name string) error {
 	if name == "" {
 		// Errors that are due to USER input should use `NewInputError` or `WrapInputError`


This comment is now incorrect and causing confusion @Naatan @MDrakos

Initial implementation of user-facing errors

ef5b08a

github-actions bot changed the base branch from master to version/0-41-0-RC1 September 1, 2023 22:09

MDrakos marked this pull request as ready for review September 1, 2023 22:20

MDrakos requested a review from Naatan September 1, 2023 22:33

Naatan requested changes Sep 6, 2023

View reviewed changes

MDrakos added 2 commits September 7, 2023 14:47

Move some error handling to errs package

08a5166

Adjust how we handle errors in example runner Add back marshaller handling in user facing error parsing

Change the error wrapping

2181e61

MDrakos requested a review from Naatan September 7, 2023 22:37

Naatan requested changes Sep 8, 2023

View reviewed changes

MDrakos added 4 commits September 8, 2023 15:36

Update reciever to be pointer

ad399cf

Add back interface Remove unused error type

Only localize user-facing errors

1670009

Use defer func to process errors

f855977

Update comment

59fd76b

MDrakos requested a review from Naatan September 11, 2023 22:16

Naatan requested changes Sep 12, 2023

View reviewed changes

Address PR review feedback

6cbaf7a

Naatan requested changes Sep 15, 2023

View reviewed changes

internal/runners/hello/hello_example.go Outdated Show resolved Hide resolved

pkg/cmdlets/errors/errors.go Show resolved Hide resolved

Address interface collision

a85fdc8

Update error tip handling

MDrakos requested a review from Naatan September 15, 2023 21:29

Naatan requested changes Sep 15, 2023

View reviewed changes

internal/errs/userfacing.go Outdated Show resolved Hide resolved

pkg/cmdlets/errors/errors.go Show resolved Hide resolved

MDrakos added 2 commits September 15, 2023 15:08

OutputError recognizes user facing errors

789635d

Rename type

3c674a1

MDrakos requested a review from Naatan September 15, 2023 22:15

Naatan requested changes Sep 15, 2023

View reviewed changes

pkg/cmdlets/errors/errors.go Outdated Show resolved Hide resolved

Add formatter conditional

1724df1

MDrakos requested a review from Naatan September 15, 2023 22:59

Naatan approved these changes Sep 15, 2023

View reviewed changes

MDrakos merged commit ef72a8c into version/0-41-0-RC1 Sep 15, 2023
6 checks passed

MDrakos deleted the DX-2151 branch September 15, 2023 23:06

mitchell-as reviewed Sep 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User-facing errors in example runner #2741

User-facing errors in example runner #2741

MDrakos commented Sep 1, 2023 •

edited by github-actions bot

Loading

Naatan Sep 6, 2023

MDrakos Sep 7, 2023 •

edited

Loading

Naatan Sep 8, 2023

Naatan Sep 6, 2023

MDrakos Sep 7, 2023

Naatan Sep 8, 2023

MDrakos Sep 8, 2023

Naatan Sep 12, 2023

MDrakos Sep 15, 2023

Naatan left a comment

Naatan Sep 12, 2023

mitchell-as Sep 26, 2023

	return JoinMessage(Wrap(e.wrapped, e.message))
	return "User Facing Error: " + e.UserError()

User-facing errors in example runner #2741

User-facing errors in example runner #2741

Conversation

MDrakos commented Sep 1, 2023 • edited by github-actions bot Loading

Choose a reason for hiding this comment

MDrakos Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDrakos commented Sep 1, 2023 •

edited by github-actions bot

Loading

MDrakos Sep 7, 2023 •

edited

Loading