Skip to content
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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

mathewmariani
Copy link

Added lever for the Squirrel programing language
(http://squirrel-lang.org/).

@mathewmariani
Copy link
Author

For my own curiosity, how does it take newly added lexers to be added into production, and usable?

lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@vidarh vidarh left a 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...

lib/rouge/lexers/squirrel.rb Show resolved Hide resolved
spec/lexers/squirrel_spec.rb Show resolved Hide resolved
@mathewmariani
Copy link
Author

I'm not sure why all the checks are failing. Can someone help me out?

@vidarh
Copy link
Contributor

vidarh commented May 12, 2019

@mathewmariani It's down to CI issues covered in #1062

@pyrmont
Copy link
Contributor

pyrmont commented Jun 1, 2019

@mathewmariani Oh noes, this looks a bit worse for wear. @miparnisari, didn't you have a problem like this? How'd you fix it?

@miparnisari
Copy link
Contributor

@pyrmont in my fork I updated the master branch against upstream and then

git checkout <branch>
git rebase -i master
git push --force

😂

@pyrmont
Copy link
Contributor

pyrmont commented Jun 1, 2019

Thanks @miparnisari!

@mathewmariani Might want to give that a try and see if that fixes things :)

@pyrmont pyrmont added the author-action The PR has been reviewed but action by the author is needed label Jun 1, 2019
@mathewmariani
Copy link
Author

Thanks @pyrmont and @miparnisari

@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jun 5, 2019
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Jun 5, 2019
Copy link
Contributor

@pyrmont pyrmont left a 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!

lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/squirrel.rb Outdated Show resolved Hide resolved
spec/visual/samples/squirrel Outdated Show resolved Hide resolved
spec/visual/samples/squirrel Outdated Show resolved Hide resolved
@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 6, 2019
@mathewmariani
Copy link
Author

@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.

@pyrmont
Copy link
Contributor

pyrmont commented Jul 5, 2019

@mathewmariani Will check the changes out and get back to you soon! :)

@pyrmont pyrmont added needs-review The PR needs to be reviewed and removed author-action The PR has been reviewed but action by the author is needed labels Jul 8, 2019
Copy link
Contributor

@pyrmont pyrmont left a 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
Copy link
Contributor

@pyrmont pyrmont Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four comments:

  1. What is the reason to have (\\.|.)? Why have the \\. in there?

  2. ["\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?

  3. 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:

    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

  4. The token for this should be Str::Double.

# strings
rule %r(@"(""|[^"])*")m, Str
rule %r("(\\.|.)*?["\n]), Str
rule %r('(\\.|.)'), Str::Char
Copy link
Contributor

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
Copy link
Contributor

@pyrmont pyrmont Jul 18, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Author

@mathewmariani mathewmariani Jul 19, 2019

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.

Copy link
Contributor

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");


#
Copy link
Contributor

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?

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jul 18, 2019
@pyrmont pyrmont self-assigned this Sep 3, 2019
@pyrmont pyrmont removed their assignment Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author-action The PR has been reviewed but action by the author is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants