Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
User-facing errors in example runner #2741
User-facing errors in example runner #2741
Changes from 1 commit
ef5b08a
08a5166
2181e61
ad399cf
1670009
f855977
59fd76b
6cbaf7a
a85fdc8
789635d
3c674a1
1724df1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This comment is now incorrect and causing confusion @Naatan @MDrakos
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.
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
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.
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 theRun
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.
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.
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.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.
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.
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.
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 thisOutputError
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 theOutputError
type.