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 ECL lexer #1396

Merged
merged 17 commits into from Feb 20, 2020
Merged

Add ECL lexer #1396

merged 17 commits into from Feb 20, 2020

Conversation

dehilsterlexis
Copy link
Contributor

Adding a lexer for ECL. ECL is the "Enterprise Control Language" for the open source HPCC-System (High Performance Computing Cluster) from LexisNexis.

@dabittan
Copy link

Well done @dehilsterlexis ! Can't wait for this to be merged and reflected in GitLab.

@pyrmont pyrmont self-assigned this Jan 17, 2020
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Jan 17, 2020
lib/rouge/demos/ecl Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
spec/lexers/ecl_spec.rb Show resolved Hide resolved
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.

Thanks for the submission. Some comments inside. Hopefully they all make sense but please let me know if anything is unclear.

lib/rouge/demos/ecl Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
spec/lexers/ecl_spec.rb Outdated Show resolved Hide resolved
@dehilsterlexis
Copy link
Contributor Author

I have made corrections, updates and additions to the files. One thing I did not solve was the EMBED but will do that in a later push. I believe I have addressed all the comments.

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review.

@dehilsterlexis
Copy link
Contributor Author

Changes made ready for review

lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/ecl.rb 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 Feb 3, 2020
@dehilsterlexis
Copy link
Contributor Author

@pyrmont for review

@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 Feb 5, 2020
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.

Two more questions:

spec/visual/samples/ecl Show resolved Hide resolved
spec/visual/samples/ecl 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 Feb 11, 2020
@dehilsterlexis
Copy link
Contributor Author

@pyrmont for review.

lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
@dehilsterlexis
Copy link
Contributor Author

@pyrmont @GordonSmith I have pushed a new version addressing all the issues and added embedding.

When I was adding embedding I found what I think to be a possible problem with the lexer. I was looking to colorize the embedding keywords and the stuff in between as a string color and encountered a case where the lexer added a word to the text. Here is how you can recreate it. The part that is commented out is what I would want to really use instead of the line above:

        rule %r(\b(?i:(beginc\+\+.*?endc\+\+)))m, Str::Single
        rule %r(\b(?i:(embed.*?endembed)))m, Str::Single
        #rule %r(\b(?i:((embed)(.)*?(endembed))))m do
          #groups Keyword, Name::Other, Keyword
        #end

The output is here. You can see it added the first token m[0] at the end of the matched phrase and colorized is as if it was the first token. It literally inserts new text. Perhaps I am doing something wrong?

RubyLexerCuriosity

@dehilsterlexis
Copy link
Contributor Author

@pyrmont nudge

@pyrmont
Copy link
Contributor

pyrmont commented Feb 17, 2020

@dehilsterlexis I've pushed some changes to the PR. Hopefully the commit messages are self-explanatory but the key change is to avoid regex interpolation as much as possible by using more generic regular expressions. Hopefully it all makes sense.

When I was adding embedding I found what I think to be a possible problem with the lexer. I was looking to colorize the embedding keywords and the stuff in between as a string color and encountered a case where the lexer added a word to the text.

I think this was because you had a nested pattern. I've fixed this in the most recent commit. If you want to use a different lexer for the embedded text, RegexLexer#delegate is the method you want. You can see some examples of how it's used in other lexers here.

lib/rouge/lexers/ecl.rb Outdated Show resolved Hide resolved
@pyrmont
Copy link
Contributor

pyrmont commented Feb 20, 2020

@ddehilster I'm ready to merge this. Is there anything left on your side?

@dehilsterlexis
Copy link
Contributor Author

@pyrmont we are good to go from our end

@pyrmont pyrmont merged commit 081edfd into rouge-ruby:master Feb 20, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Feb 20, 2020

@dehilsterlexis Merge successful! We're on a monthly release cadence and the next version is scheduled to ship on Tuesday 10 March. This will be part of that release. Thanks for the contribution!

@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Feb 20, 2020
@dabittan
Copy link

@pyrmont , is the ECL lexer live yet? we still don't see it reflected in our GitLab ecl code.

@pyrmont
Copy link
Contributor

pyrmont commented Mar 11, 2020

@dabittan The ECL lexer is part of Rouge v3.17.0 (dingus). I'm not associated with GitLab and don't know what version of Rouge they are running.

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.

None yet

6 participants