-
Notifications
You must be signed in to change notification settings - Fork 12
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
Improve config delimiters characters check #180
Conversation
52a55d3
to
fb089fe
Compare
"> and <+b+>" | ||
--> tests/ui/broken-config.rs:20:21 | ||
| | ||
20 | #[template(source = "<+a+> and <+b+>", config = "operator-plus-config.toml", syntax = "plus", ext = "txt")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in here, the template is perfectly valid but doesn't work. Not sure how to fix that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! It's much better with your change, to look for an offending character than to enumerate all combinations that could contain this character.
25cecd0
to
daa93c2
Compare
Finally had some time to update this. I added the missing operators, thanks for the suggestion! |
rinja_parser/src/lib.rs
Outdated
@@ -824,6 +824,15 @@ impl<'a> SyntaxBuilder<'a> { | |||
"delimiters may not contain white spaces. \ | |||
The {k} delimiter ({s:?}) contains white spaces", | |||
)); | |||
} else if is_closing | |||
&& ['(', ')', '-', '+', '~', '.', '>', '<', '&', '|', '!'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, )
should be fine. We only ever look for )
in conjunction with a (
. This way, if someone wants to use e.g. (( expr ))
it would still work fine. Sorry that I didn't notice that during my first review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
daa93c2
to
c77cd37
Compare
Updated. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, thanks! :)
Finally I have something before the next release. :)