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: Change annotation syntax to use @ #2729

Merged
merged 4 commits into from
Jun 5, 2023
Merged

Conversation

max-sixty
Copy link
Member

This is not great code — likely needs some error handling, and maybe some changes to how it's implemented in Chumsky. But it works!

This is not great code — likely needs some error handling, and maybe some changes to how it's implemented in Chumsky. But it works!
@snth
Copy link
Member

snth commented Jun 5, 2023

Sorry, there's no linked issue so asking here: was there a discussion around #[...] vs @{...} syntax for annotations?

No preference from my side at this stage but just curious what the considerations would be.

Copy link
Member

@aljazerzen aljazerzen left a comment

Choose a reason for hiding this comment

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

Makes sense. Annotations can now be arbitrary expressions - literals, function calls, tuples, lists.

And SQL backend expects only tuples.

@aljazerzen
Copy link
Member

aljazerzen commented Jun 5, 2023

We mentioned here: #2694

@snth
Copy link
Member

snth commented Jun 5, 2023

Some thoughts that came to me on this are that

  • to the extent that the annotations are key-value pairs or tuples, it makes that we use our {} syntax for those, and
  • if you think of them as ATtributes, then it makes sense to use the @ symbol as a useful mnemonic.

So @{...} seems like a great choice.

@@ -7,5 +7,5 @@ SELECT
FROM
tracks
WHERE
name ~ '\(I Can''t Help\) Falling'
(name) ~ ('\(I Can''t Help\) Falling')
Copy link
Member

Choose a reason for hiding this comment

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

These changes are intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I saw these too — I'm not sure why they happen...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry!

(I blame the comment syntax :D )

@max-sixty
Copy link
Member Author

I cleaned up the code a bit, and merging.

I don't know why we're getting these extra parentheses at #2729 (comment) — I don't even see where it could be affecting this! Not super important so I won't spend time on it, lmk any dissent

@max-sixty max-sixty enabled auto-merge (squash) June 5, 2023 18:42
@max-sixty max-sixty merged commit 55a6bf8 into PRQL:main Jun 5, 2023
@max-sixty max-sixty deleted the annotate branch June 5, 2023 18:45
@vanillajonathan
Copy link
Collaborator

In the future could there be other types of annotations?
Could custom annotations be declared?

Maybe @Binding{Strength=4} would be preferred over @{binding_strength=11} in that case.

The syntax is rather unorthodox, hence I think using square brackets instead of curly brackets would be preferred as it would be more familiar.

@aljazerzen
Copy link
Member

Syntax for annotations is just:

Annotation :== '@' Expr

... where Expr can be any expression. We are just using a tuple here. You could use [] and have arrays.

In any case, this was added mostly as an internal detail. It is not documented and especially binding_strength is not part of the language, so it can be changed without notice. So there is no need to make it future proof.

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.

5 participants