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

feat(diagnostics): improve assignment fallibility compilation error #553

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

pront
Copy link
Member

@pront pront commented Nov 13, 2023

closes: #23

@pront pront requested a review from fuchsnj November 13, 2023 14:31
@pront pront enabled auto-merge November 13, 2023 17:23
@pront pront added this pull request to the merge queue Nov 13, 2023
Merged via the queue into main with commit 1aef3d3 Nov 13, 2023
9 checks passed
@pront pront deleted the pront/OPW-89-fallible-assignment-human-friendly-error branch November 13, 2023 17:32
Comment on lines 10 to 14
# │ │ this expression is fallible
# │ │ update the expression to be infallible
# │ │ note if an argument type is invalid it can render a function fallible
# │ │ '.result[0].an' argument type is 'string or undefined' and this function expected a parameter 'value' of type 'string'
# │ or change this to an infallible assignment:
# │ .a, err = sha3(.result[0].an)
Copy link
Member

Choose a reason for hiding this comment

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

#   │ │    this expression is fallible because at least one argument's type cannot be verified to be valid
#   │ │    '.result[0].an' argument type is 'string or undefined' and this function expected a parameter 'value' of type 'string'
#   │  update the expression to be infallible by adding a `!`: `.a = sha3!(.result[0].an)`
#   │ or change this to an infallible assignment:
#   │ .a, err = sha3(.result[0].an)

What you have here is a great step forward, but do you think we can make it even clearer with something like what I put? I still worry that the current error message will be confusing to users. I'm not sure exactly what's possible though.

Copy link
Member Author

@pront pront Nov 13, 2023

Choose a reason for hiding this comment

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

If I read the above correctly you want to add two more lines:

this expression is fallible because at least one argument's type cannot be verified to be valid
// ...
update the expression to be infallible by adding a `!`: `.a = sha3!(.result[0].an)`

which should be doable and I will follow up on this.

Ultimately users should have some basic understanding about fallibility and fail safety when using VRL (which practically means adding a ! or handling the returned err).

P.S. Where users might still be confused, is with code blocks which contain fallible expressions example here. However, that part of the codebase is not easy to maintain and is need of refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

#   │ │    this expression is fallible because at least one argument's type cannot be verified to be valid
#   │ │    '.result[0].an' argument type is 'string or undefined' and this function expected a parameter 'value' of type 'string'

is actually the more important update I would like to see. I think my verbiage makes it more clear exactly what the issue is: that the function is fallible because one of the arguments cannot be verified, and this is the argument (or arguments) in particular, their types, and what the function expects.

The hints on how to fix are then a cherry on top.

Copy link
Member

Choose a reason for hiding this comment

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

The hints could also include casting the arguments like:

#   │ .a = sha3(string!(.result[0].an))

I think that is closer to the "best-practice" that we want to guide people into.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, this makes sense.

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.

Add note to error diagnostics when fallibility is due to function arguments
3 participants