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 fallibility error notes #523

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

pront
Copy link
Collaborator

@pront pront commented Oct 24, 2023

part of #23

@pront pront requested a review from fuchsnj October 24, 2023 21:41
@@ -9,10 +9,12 @@
# │ │ │
# │ │ this expression is fallible
# │ │ update the expression to be infallible
# │ │ note if an argument type is invalid it can render a function fallible
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be great if we could go even further and say, for cases where the function is fallible due to the argument type not being known to be valid, that:

this expression is fallible because argument `.result[0].an` is of type `any` and this function expects a `string`. It will fail at runtime if `.result[0].an` is not a `string`

The currently added message is definitely an improvement, but I think users will remain largely confused by this behavior of VRL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jszwedko: Agreed, good point.

However, I don't have time to do it before this upcoming release. Two options:

  1. Get this improvement in and follow up with another PR (to be included in the next release)
  2. Leave this PR open

What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

👍 I'm fine with merging this since it is an improvement and then following up. I just want to make sure we don't close out #23 until we do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. Added a reminder here.

Merging this PR won't close the ticket, so if you give this a ✅ I will include it in the VRL release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was more involved than expected but implemented this here: #553

@pront pront marked this pull request as draft October 30, 2023 13:20
@pront pront marked this pull request as ready for review October 30, 2023 20:08
@pront pront requested a review from jszwedko October 30, 2023 20:08
@pront pront enabled auto-merge October 30, 2023 20:09
@pront pront added this pull request to the merge queue Oct 30, 2023
Merged via the queue into main with commit 8fbf561 Oct 30, 2023
10 checks passed
@pront pront deleted the pront/fallability-diag-enhancements branch October 30, 2023 21:30
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.

2 participants