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

CM KCL: add annotations #5314

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

mattmundell
Copy link
Contributor

@mattmundell mattmundell commented Feb 7, 2025

What

In codemirror-lang-kcl, add support for annotations to the Lezer parser.

I've done this the simple way by treating the annotation as a statement and allowing space, eg between the @ and setting and (. If it really needs to parse the spacing correctly in future it might need to use Lezer's external tokenizer feature.

Note that the !annotation forces the parser to choose the AnnotationList, because at that point the ( could also be the start of a statement.

Why

Prepares for adding highlights to annotations.

References

Related: /issues/5204.

Copy link

qa-wolf bot commented Feb 7, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Feb 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Feb 11, 2025 9:00am

@jtran
Copy link
Collaborator

jtran commented Feb 10, 2025

allowing space, eg between the @ and setting and (.

Is it possible to do something similar to identifier?

@mattmundell
Copy link
Contributor Author

Is it possible to do something similar to identifier?

It's kind of possible, but there are issues:

  • if you make @annotation( a token like identifier then the bracket matching will stop working, and the ( and @ will always be highlighted the same as annotation. And the () is optional so you have to deal with that somehow.
  • if you make @annotation the token then you still have the problem that the parser will accept space before the (. And you lose the ability to highlight the @ separately.

I think the right way to do this is to use @external token to parse the @annotation( into three separate tokens while ignoring the whitespace @skip. But that's more complex and I think for now the easy way will catch everything.

@jtran
Copy link
Collaborator

jtran commented Feb 11, 2025

Thank you for the explanation. I suspect that it's an oversight that whitespace isn't allowed between @settings and ( since it is allowed for regular function calls. I think it's okay to highlight @settings as a single token, not @ separately, because annotations are essentially in their own namespace. I believe you can define fn settings() { return 0 } and you can use it without colliding with @settings. For this reason, I would go with your second bullet's approach.

Related: the name after @ and before ( is optional as of #5324.

@mattmundell
Copy link
Contributor Author

635d5fd goes with the second bullet and 4054e6e aligns with #5324.

This will need some adjustment when adding support for unlabeled args in fn definitions, which also uses @.

About the space before the (, the Rust parser looks to be treating annotations like block comments, so maybe there were/are plans to allow annotations anywhere, like fn x @eg () { return 1 }. Prohibiting the space would prevent ambiguity in that example.

@jtran
Copy link
Collaborator

jtran commented Feb 11, 2025

There are plans to allow annotations anywhere.

Yes, it currently conflicts with the unlabeled arg in function definitions. I don't think we decided how to resolve that yet. People seemed to be leaning towards changing how unlabeled args are defined.

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