-
Notifications
You must be signed in to change notification settings - Fork 740
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
Add support for labels to Kotlin lexer #1496
Conversation
@goodhoko Sorry about the confusion. This should have worked but unfortunately didn't because of a longstanding bug in the StringScanner library that Rouge uses. As a result of the bug, lookbehinds don't work :( That bug has been fixed but enabling the fix is opt-in and we haven't enabled that yet (see #1491 for more details). In any event, the simplest fix is to match the terms in the lookbehind earlier in the state. That's what I've done with the latest commit. Let me know how it looks on your end. |
@pyrmont thanks for the link it all makes sense now. Also thanks for your commit. It's completely ok from my side so we can merge, possibly squashing the commits? .) By the way I noticed that there's quite a lot of look behind groups in rouge lexers.
Does this mean they all don't work? |
Uh, maybe wait a little, I'll also add some tests for the labels to the lexer spec. |
@pyrmont OK, LGTM. Should be squashed to have feature and tests in one commit. 👍 // I come from GitLab where squashing is a single click before merging. Let me know if I need to do this manually here. |
@goodhoko I think that yes, those rules probably don't match precisely as intended. That said, in most cases, I'd expect it results in syntactically incorrect code lexing without problems rather than syntactically correct code having errors. Since syntax correctness is a non-goal of Rouge that's not the biggest problem (although it is something to consider when we move to the improved StringScanner). In any event, thank you for adding the tests. These all look good—will merge in now (our merging strategy is squash and merge when we accept PRs so nothing for you to do!). |
@goodhoko OK, merged in now. Thanks for the PR! This will be part of the next release of Rouge. That's scheduled for Tuesday 14 April which, depending on where you are when you read this, is either today or tomorrow 🎉 |
This commit adds support for labels to the Kotlin lexer.
Rouge will currently match identifiers in the Kotlin lexer that begin with certain keywords (such as `super`). This is because of a change in #1496 that added a rule to support labels. That change should have updated the rule to end with a `\b` so as to ensure that identifiers that begin with the listed keywords are followed by a word break. This commit fixes that error.
Rouge will currently match identifiers in the Kotlin lexer that begin with certain keywords (such as `super`). This is because of a change in rouge-ruby#1496 that added a rule to support labels. That change should have updated the rule to end with a `\b` so as to ensure that identifiers that begin with the listed keywords are followed by a word break. This commit fixes that error.
Hello,
I'm trying to add support for labels to the Kotlin lexer. I managed to get it work for the label definition. Eg.
but can't make it work for the rule for label reference. Eg.
I suspect the look behind group is the cause but I'm not really sure. I'd like to ask for help on this. .)
For reference how labels are used in Kotlin see the Break and Continue Labels.