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

[ExpressionLanguage] Add comment support to expression language #54978

Open
wants to merge 1 commit into
base: 7.2
Choose a base branch
from

Conversation

valtzu
Copy link
Contributor

@valtzu valtzu commented May 17, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
License MIT

When expression language expressions are provided by the user, it would be really helpful if we could include comments in the expression. Many times the user will write something like

customer.group == 'good_customer' or customer.id == 123

which in my opinion, would be much better with a comment

customer.group == 'good_customer'
or customer.id == 123 /* 123 is the test customer */

so that the next person reading the expression knows what the magical 123 is.


The implementation is quite simple, given it's ok to strip the comments from AST & the compiled version.

Why /* & */

This seems to be the most common way among different programming languages, including JS, C#, Go, C, PHP, Java, Rust – also in business scenarios you often times want multi-line comments, for which this syntax usually stands for. Also, this makes it possible to use comments in the middle of single-line expressions.


At the moment, unlike multi-line comments in PHP, we always require closing */.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "7.2".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@OskarStark OskarStark modified the milestones: 7.1, 7.2 May 17, 2024
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

This feature is useful when used with user input where it is not otherwise possible to add comments in the code.

I think you could add tests on expressions like /* /* comment */ and /* comment */ */ to ensure the regex catch the right comment separators.

@valtzu valtzu force-pushed the expression-language-add-comments branch from d5ce011 to c762141 Compare May 18, 2024 09:08
@valtzu
Copy link
Contributor Author

valtzu commented May 18, 2024

Added some extra tests + changelog (too late for 7.1 as I understand)

@valtzu valtzu force-pushed the expression-language-add-comments branch from c762141 to d50ca10 Compare June 2, 2024 12:27
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.

None yet

5 participants