fix(ruby) symbols, string interpolation, class names with underscores #4213
Conversation
jimtng
force-pushed
the
ruby-class-underscore
branch
from
169267e to
f1ae3ab
Compare
jimtng
changed the title
jimtng
force-pushed
the
ruby-class-underscore
branch
3 times, most recently
from
39e0632 to
5e38774
Compare
jimtng
changed the title
Signed-off-by: jimtng <[email protected]>
jimtng
force-pushed
the
ruby-class-underscore
branch
from
5e38774 to
67644b9
Compare
| const CLASS_NAME_WITH_NAMESPACE_RE = regex.concat(CLASS_NAME_RE, /(::\w+)*/) | ||
| const CLASS_NAME_RE = /\b([A-Z]+[a-z0-9_]+)+[A-Z]*/; |
There was a problem hiding this comment.
Doesn't this removal break (at least) the inheritance highlighting?
There was a problem hiding this comment.
Can you give an example syntax of what you mean?
Class::With::Namespacewill work fine and better, because it doesn't highlight the :: much like it doesn't here in github, or vscode.
There was a problem hiding this comment.
class C::E < A::B
end
We have special rules to match syntax where we KNOW an identifier is a class (or class with namespaces).
it doesn't here in github, or vscode
Striving for exact matches against other highlighting tools isn't a thing we care about here.
In this case I'm sympathetic to not highlighting the ::, but only if it can be done without adding a lot of complexity or advanced parsing to the grammar.
There was a problem hiding this comment.
You really want something like [class](::[class])* but our multi-match engine can't really handle that - with discrete coloring of the different pieces. Maybe if you tried a long sequence with some items that matched 0 length, but I'm not sure I've tested multi-matching in that type of scenario before.
There was a problem hiding this comment.
This case would've worked fine, except it didn't recognise single-letter classes. I've adjusted it to do so.
class C::E < A::B
end
There was a problem hiding this comment.
Please use "show the structure". as you develop. If you're matching all those "one off" with just your class rule then you're missing out on the fact that the portion after the < is not just a title.class, it's a title.class.inherited.
This is why the simple multi-match rules exist - to flesh out more detail as well as handle cases like class A where we KNOW A is a class here, not a constant (as it would be with non other context).
There was a problem hiding this comment.
I wasn't aware of the different handling for title.class.inherited.
Whilst special casing class A works using a multi-match rule, it wouldn't help for A.method(foo) which will still be matched as a constant. So I'd suggest we simplify and not handle the class A case at all.
In the current version, it doesn't work either: https://highlightjs.org/demo#lang=ruby&v=1&theme=atom-one-dark&code=Y2xhc3MgQQplbmQKCscNOjpCywsgPCBDOjpExxJGb286OkZvbyA8yQvKGsYVyEjEC8VXQSA9ICdYJw%3D%3D
There was a problem hiding this comment.
With the latest commit here, it looks like this
src/languages/ruby.js
Outdated
| { | ||
| className: 'symbol', | ||
| begin: hljs.UNDERSCORE_IDENT_RE + '(!|\\?)?:', | ||
| relevance: 0 | ||
| }, | ||
| { | ||
| className: 'symbol', | ||
| begin: ':(?!\\s)', | ||
| begin: '(?<!:):(?!\\s|:)', |
There was a problem hiding this comment.
We can't use negative lookbehind until v12 as that would be a breaking change.
There was a problem hiding this comment.
The rule on line 378 is designed to deal with :: and prevent false symbol positives - until v12 where it would be simpler to use negative lookbehind.
There was a problem hiding this comment.
Removing this negative lookbehind will cause this:
The rule on line 378 doesn't seem to do its job.
So I had to adjust line 378 so I can remove the negative lookbehind.
Signed-off-by: jimtng <[email protected]>
Signed-off-by: jimtng <[email protected]>
jimtng
force-pushed
the
ruby-class-underscore
branch
from
23fc7dd to
0be49c6
Compare
| @@ -375,7 +375,7 @@ export default function(hljs) { | |||
| METHOD_DEFINITION, | |||
| { | |||
| // swallow namespace qualifiers before symbols | |||
| begin: hljs.IDENT_RE + '::' | |||
| begin: '::' | |||
Signed-off-by: jimtng <[email protected]>
Signed-off-by: jimtng <[email protected]>
|
@joshgoebel can we use a positive lookbehind? It seems to work when I tested it. |
Signed-off-by: jimtng <[email protected]>
Signed-off-by: jimtng <[email protected]>
Signed-off-by: jimtng <[email protected]>
No. Until v12 we have to support older versions of Safari that don't have this built-in. What issue are we still trying to solve with lookbehind? |
I used a positive lookbehind for |
Signed-off-by: jimtng <[email protected]>
|
@joshgoebel I've managed to do the inheritance without the use of a lookbehind 
|
Also fix #3676
Change from:
To:
Checklist
CHANGES.md