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

Handle non-existing variable in comparison #676

Merged
merged 4 commits into from
Jul 12, 2023

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Jul 6, 2023

Description

Before when we encountered a non-existing variable in a comparison it would result in an error: "no variable found for name 'x'".
This commit changes this so we can deal with non-existing variables. They now get treated as ValNull and the comparison succeeds without errors.

Related issues

Fixes part of #582

Adds a few test cases where we do comparisons on non-existing variables.
Before when we encountered a non-existing variable in a comparison it would result in an error: "no variable found for name 'x'".
This commit changes this so we can deal with non-existing variables. They now get treated as ValNull and the comparison succeeds without errors.
Expectations of these existing test cases were wrong. They would expect errors or booleans as a result, where we now return a ValNull.

Not all behaviours were clear! As a result some test cases have been ignored until this is clarified.
@remcowesterhoud remcowesterhoud force-pushed the 582_non_existing_variables_in_comparison branch from 86e4c8c to c38488f Compare July 7, 2023 09:48
@remcowesterhoud remcowesterhoud marked this pull request as ready for review July 7, 2023 09:49
@remcowesterhoud remcowesterhoud requested a review from saig0 as a code owner July 7, 2023 09:49
Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@remcowesterhoud awesome. 🎉

I like that the new tests are in a separate test class. 👍

Before merging, please add one more test case for the between operator.

Adds test cases verifying a comparison with a non-existing variable using `between x and y` will result in ValNull
@remcowesterhoud
Copy link
Contributor Author

I have added between to the testcases. I'll merge this now and create the follow-up issues you mentioned.

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