-
Notifications
You must be signed in to change notification settings - Fork 745
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
Added Squirrel lexer #814
base: master
Are you sure you want to change the base?
Added Squirrel lexer #814
Conversation
Added lever for the Squirrel programing language (http://squirrel-lang.org/).
For my own curiosity, how does it take newly added lexers to be added into production, and usable? |
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.
Hi, I'm trying to help triage old pull requests so we can work down the backlog. It looks like you've addressed the previous comments. Just one more nitpick - '# frozen_string_literal: true' was added project wide since the PR was open. If you please add that, and then squash your commits, I think this one looks ready to go...
Added lever for the Squirrel programing language (http://squirrel-lang.org/).
I'm not sure why all the checks are failing. Can someone help me out? |
@mathewmariani It's down to CI issues covered in #1062 |
Added lever for the Squirrel programing language (http://squirrel-lang.org/).
Added lever for the Squirrel programing language (http://squirrel-lang.org/).
@mathewmariani Oh noes, this looks a bit worse for wear. @miparnisari, didn't you have a problem like this? How'd you fix it? |
@pyrmont in my fork I updated the master branch against
😂 |
Thanks @miparnisari! @mathewmariani Might want to give that a try and see if that fixes things :) |
Thanks @pyrmont and @miparnisari |
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.
This is looking pretty good, @mathewmariani :) Let me know if any of the comments are unclear!
@pyrmont I have re-written the lexer. The original merge-request is almost two years old, and I felt I have learned more about Ruby since then. |
@mathewmariani Will check the changes out and get back to you soon! :) |
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.
Sorry it took a bit longer to get to this than I'd hoped. Overall, it's looking good! I think we're pretty close now :)
|
||
# strings | ||
rule %r(@"(""|[^"])*")m, Str | ||
rule %r("(\\.|.)*?["\n]), Str |
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.
Four comments:
-
What is the reason to have
(\\.|.)
? Why have the\\.
in there? -
["\n]
is a character class and will match against any character in the class. This regex will match against a string that begins with"
and ends in a newline even if there was no matching"
. Was that the intention? -
This pattern will mean that the lexer won't highlight escape sequences in strings (eg.
\n
). The Swift lexer is an example of how this can be done:rouge/lib/rouge/lexers/swift.rb
Lines 163 to 170 in b6411b1
state :dq do rule %r/\\[\\0tnr'"]/, Str::Escape rule %r/\\[(]/, Str::Escape, :interp rule %r/\\u\{\h{1,8}\}/, Str::Escape rule %r/[^\\"]+/, Str rule %r/"""/, Str, :pop! rule %r/"/, Str, :pop! end -
The token for this should be
Str::Double
.
# strings | ||
rule %r(@"(""|[^"])*")m, Str | ||
rule %r("(\\.|.)*?["\n]), Str | ||
rule %r('(\\.|.)'), Str::Char |
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.
Same issue here as with the above.
rule %r(/[*].*?[*]/)m, Comment::Multiline | ||
|
||
# strings | ||
rule %r(@"(""|[^"])*")m, Str |
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.
I think the token for this should be Str::Heredoc
.
rule %r((?:class)\b), Keyword::Declaration, :class | ||
|
||
# operators and punctuation | ||
rule %r([~!%^&*()+=|\[\]{}:;,.<>\/?-]), Punctuation |
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.
When used in a character class, -
should either be escaped or be the first character in the class.
rule %r([~!%^&*()+=|\[\]{}:;,.<>\/?-]), Punctuation | ||
|
||
rule %r([0-9]+(([.]([0-9]+)?)(e[-]?[0-9]+)?)), Num::Float | ||
rule %r(0x[0-9a-fA-F]+|0[0-7]+|[0-9]+), Num::Integer |
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.
The first part of this pattern should be broken out into a separate rule to match against the hexadecimal numbers. The token for that would be Num::Hex
.
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.
I decided to concatenate them since Squirrel reads Hexadecimals as Integers. But, I can separate them.
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.
Yeah, the names of the tokens are really a little misleading.
All languages I know (all languages maybe?) use different numeric bases only for input. Once a number is input, it's whatever it is: an integer, a float, etc. The token Num::Integer
should really be Num::Integer::Decimal
and Num::Hex
should be Num::Integer::Hex
. Alas, Pygments chose the tokens that they did.
Could you separate the hexadecimal representation pattern into a new rule with a new token? That will be consistent with the other lexers.
print(fib(5) + "\n"); | ||
|
||
|
||
# |
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.
Could you add an example (possibly here) of the multiline comment?
Added lever for the Squirrel programing language
(http://squirrel-lang.org/).