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

Refactor when expression handling #600

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Conversation

mjpieters
Copy link
Contributor

  • Use expr.Patch and a visitor to only replace identifiers that start with $ with a Env. member lookup.
  • use expr.AsBool() to assert the expression produces a boolean
  • Clean up envMap parsing to use strings.Cut()
  • Expand tests, using testing.T.Run() with test names.

Fixes #599

@mjpieters
Copy link
Contributor Author

mjpieters commented Jun 21, 2024

There are a few changes in behavior:

  • using undefined variables in an expression now provides a much friendlier error:

    $ tbls --when 'foo'        
    unknown name foo (1:1)
     | foo
     | ^
    

    Before you got:

    $ tbls --when 'foo'
    cannot fetch foo from struct { Env map[string]string } (1:2)
     | (foo) == true
     | .^
    
  • You now also get an error when the expression isn't going to produce a boolean value:

    $ tbls --when '$SHELL'
    expected bool, but got string
    

    Without this PR, there was no error, the test simply failed because no string is ever equal to true.

  • Accessing an environment variable that doesn't exist results in an empty string (the default 'zero' value for the string type):

    $ tbls --when '$NONESUCH != ""'
    

    where before this would be a (cryptic) error:

    $ tbls --when '$NONESUCH != ""'
    cannot fetch $NONESUCH from struct { Env map[string]string } (1:2)
    | ($NONESUCH != "") == true
    | .^
    

    You can use in and not in to test if a variable is defined, instead:

    $ tbls --when '"NONESUCH" in Env`
    

- Use expr.Patch and a visitor to only replace identifiers that start
  with `$` with a `Env.` member lookup.
- use `expr.AsBool()` to assert the expression produces a boolean
- Clean up envMap parsing to use strings.Cut()
- Expand tests, using testing.T.Run() with test names.
@k1LoW
Copy link
Owner

k1LoW commented Jun 24, 2024

@mjpieters Looks Great!!! thank you!!!

@k1LoW k1LoW added the minor label Jun 24, 2024
@k1LoW k1LoW merged commit 157411d into k1LoW:main Jun 24, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

when expression parsing too simplistic
2 participants