Skip to content

fix!: fix macros with spaces to make them more consistent#4052

Merged
tobymao merged 1 commit intomainfrom
toby/macros_with_spaces
Mar 31, 2025
Merged

fix!: fix macros with spaces to make them more consistent#4052
tobymao merged 1 commit intomainfrom
toby/macros_with_spaces

Conversation

@tobymao
Copy link
Contributor

@tobymao tobymao commented Mar 28, 2025

this could break existing macro usage with variables that have spaces

@tobymao tobymao requested review from a team, georgesittas and izeigerman March 28, 2025 22:04
Comment on lines +1103 to +1093
evaluator.evaluate(d.parse_one(""" @DEF(x, "a b") """))
evaluator.evaluate(d.parse_one(""" @DEF(y, 'a b') """))
evaluator.evaluate(d.parse_one(""" @DEF(z, a."b c") """))

for sql, expected in (
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider testing with config variables as well besides @DEF ones. For example:

# config.yaml
variables:
  v1: "a b"
  v2: '"a b"'
  v3: "'a b'"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure if config variables iare in the scope of these tests, that's probably something to go with your changes

this could break existing macro usage with variables that have spaces
@tobymao tobymao force-pushed the toby/macros_with_spaces branch from deb9277 to 114bb9b Compare March 31, 2025 02:42
@tobymao tobymao enabled auto-merge (squash) March 31, 2025 02:42
@tobymao tobymao merged commit d015490 into main Mar 31, 2025
22 checks passed
@tobymao tobymao deleted the toby/macros_with_spaces branch March 31, 2025 02:53
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.

3 participants