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

Add HTML::Mason lexer #838

Merged
merged 13 commits into from
Jul 19, 2019
Merged

Conversation

miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Dec 17, 2017

HTML::Mason is a templating framework, much like ASP.NET Razor. Basically the syntax is this: everything is considered to be HTML code unless a special block, a comment, a substitution tag, a component call, or a line that begins with % is encountered.

See below how the result of this PR compares against IntelliJ's Perl5 plugin.

image

@miparnisari miparnisari changed the title [do not merge] Add HTML::Mason lexer Add HTML::Mason lexer Dec 30, 2017
@gfx
Copy link
Member

gfx commented Jan 31, 2018

@miparnisari

Thanks for the contribution, but this lexer can't handle the following snippet:

<%args>
    $x = "</%args>"
</%args>

Rouge can handle nested syntax, although there are a few docs. https://github.com/jneen/rouge/blob/master/lib/rouge/lexers/erb.rb might help you.

@miparnisari miparnisari force-pushed the add-mason-lexer branch 2 times, most recently from 9194bbf to 9ebd6d6 Compare February 4, 2018 09:11
@miparnisari
Copy link
Contributor Author

@gfx

Thanks for the feedback! I've updated the sample and the lexer.

@dblessing
Copy link
Collaborator

@miparnisari I am not familiar with this framework so forgive me if this doesn't make any sense. Would it be helpful to subclass the HTML lexer so you don't have to rewrite the HTML-specific pieces? Then simply add on the framework highlighting on top?

@miparnisari
Copy link
Contributor Author

@dblessing

I changed this lexer to inherit from TemplateLexer. There are some things that this lexer can't handle, like the following CSS

  .dashed {
        background: url("<% $someUrl %>");
   }

But I can live with that. I'm not going to change the CSS lexer for this.

@miparnisari
Copy link
Contributor Author

@dblessing any chance of merging this soon?

@miparnisari miparnisari force-pushed the add-mason-lexer branch 3 times, most recently from 12ddf8e to 482693c Compare November 3, 2018 04:14
@vidarh
Copy link
Contributor

vidarh commented Jan 24, 2019

@miparnisari All of your outstanding pull-requests are on my list of stuff to go through, so sure.. I just went for the easy/low hanging fruit first (which mostly meant the ones with little previous discussion).

@pyrmont
Copy link
Contributor

pyrmont commented May 29, 2019

@miparnisari It might be worth creating a new branch from the current master, adding your changes to that and then pushing a new PR. This one's now got a whole lot of irrelevant files that'll make things more difficult to review in the future.

@miparnisari
Copy link
Contributor Author

@pyrmont i screwed up while updating my fork haha. But it should be good now, there's only 6 files changed

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label May 29, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jun 10, 2019

Will try to have a look at this one soon, @miparnisari!

@miparnisari
Copy link
Contributor Author

@pyrmont any news? :)

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, I forgot this one was outstanding (which is supposed to be the point of the needs-review tag, I realise!).

In addition to the comments here, we've now moved to use rule %r/.../ rather than rule /../ to keep RuboCop happy. Could you update these as well? Rules in the form of rule %r(...) are fine and don't need to be updated.

lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
spec/visual/samples/mason Show resolved Hide resolved
spec/visual/samples/mason Show resolved Hide resolved
spec/visual/samples/mason Show resolved Hide resolved
spec/visual/samples/mason Outdated Show resolved Hide resolved
spec/visual/samples/mason 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 28, 2019
@miparnisari
Copy link
Contributor Author

@pyrmont I tried to update rules like rule(/(.*?)(?=%>)/m) { delegate @perl } to use the rule %r syntax but couldn't get it work. I'm probably too sleepy to function

@ashmaroli
Copy link
Contributor

I tried to update rules like.. but couldn't get it work.

@miparnisari Have you tried keeping the parentheses:

- rule(/(.*?)(?=%>)/m) { delegate @perl }
+ rule(%r/(.*?)(?=%>)/m) { delegate @perl }

Either ways, the primary reason for adopting the %r syntax was to remove ambiguity warnings from the interpreter regarding constructs such as rule /\w+/ because the / can be seen as a division operator as well.. (??)
But since inline blocks here have parentheses (rule(/(.*?)(?=%>)/m) { delegate @perl }), it shouldn't output a warning at all..

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.

Oops! I marked the previous review as approved and then of course noticed something I wanted to comment on.

spec/visual/samples/mason Show resolved Hide resolved
@pyrmont
Copy link
Contributor

pyrmont commented Jun 30, 2019

@miparnisari Yep, @ashmaroli is correct. You don't need to worry about adding the %r bit to rule(...)-style rules. What you did looks fine :)

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

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Some minor changes recommended..

lib/rouge/guessers/disambiguation.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
@pyrmont pyrmont added dependent-pr The PR is dependent on other changes being merged and will be reviewed later. author-action The PR has been reviewed but action by the author is needed and removed author-action The PR has been reviewed but action by the author is needed dependent-pr The PR is dependent on other changes being merged and will be reviewed later. labels Jul 16, 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.

One more change, sorry!

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, one last change!

lib/rouge/lexers/mason.rb Outdated Show resolved Hide resolved
@pyrmont pyrmont merged commit d866778 into rouge-ruby:master Jul 19, 2019
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jul 19, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jul 19, 2019

Hey, look! It's finally merged! :D

@miparnisari
Copy link
Contributor Author

Thanks so much @pyrmont !!! Best way to start my Friday :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants