-
-
Notifications
You must be signed in to change notification settings - Fork 14
Support tilde token for concatenation of string literals #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
Conversation
1 similar comment
djc
left a comment
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.
Thanks for the PR! This should definitely support ~, I just hadn't come across anything using it yet (and I may have missed it when reading the spec). I requested a few minor changes, are you up for making those? Otherwise I can just fix things up myself.
Additionally, I'd prefer to fold the first and last commits into the relevant other commits so I have nicer commit history, but I can take care of that easily if you don't want to get into that.
| 'LBRACE', 'LBRACKET', 'LPAREN', 'LIST', 'LITERAL', 'MINUS', 'MIXED', | ||
| 'PLUS', 'PIPE', 'QMARK', 'RBRACE', 'RBRACKET', 'RPAREN', 'STAR', 'TILDE', | ||
| ] + [s.upper() for s in KEYWORDS]) | ||
| ] + [s.upper() for s in KEYWORDS], precedence=[("left", ['TILDE'])]) |
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.
Is the explicit precedence here actually needed?
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.
Yes, I think so. Although I'm not sure of a better way to avoid a conflict.
rnc2rng/parser.py
Outdated
|
|
||
| @pg.production('strlit : strlit TILDE strlit') | ||
| def primary_concat(s, p): | ||
| p[0].value = p[0].value + p[2].value |
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.
Doing += here would be slightly nicer, I think, do you agree?
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.
For sure!
|
Great, thanks! I made the operator change (left a comment on the first) and squashed commits. |
|
Merged! I've also pushed a new release to PyPI, thanks. I'd like to add you to the AUTHORS file if you don't mind passing me a real email address (or make a PR yourself). |
|
Sure! Address is wamuir@gmail.com |
|
Added in d96073e. |
My read of the RELAX NG Compact Syntax spec indicates that two string literals should be concatenated when immediately surrounding a tilde. This is not currently supported by rnc2rng but could be useful. As one example, Appendix B to RFC 4278 (ATOM syndication) utilizes several tildes and concatenation would be expected. PR to include support in parser.py and add a simple test.