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 SAS lexer and accompanying files #1107

Merged
merged 14 commits into from
Jul 17, 2019
Merged

Conversation

tomsutch
Copy link
Contributor

@tomsutch tomsutch commented May 1, 2019

Add lexer for SAS.

It's a bit of an odd language with lots of keywords, most of them specific to individual procedures, so I hope I have gone about this in an appropriate way. I haven't attempted to implement all of the procedure-specific stuff by any means.

Happy to change in response to feedback :-)

pyrmont added a commit to pyrmont/rouge that referenced this pull request May 10, 2019
Import: Add SAS lexer and accompanying files rouge-ruby#1107
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label May 28, 2019
@pyrmont
Copy link
Contributor

pyrmont commented May 29, 2019

Forgive me, @tomsutch. It might take a bit of time to get through this lexer—there sure are a lot of keywords!

lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/sas.rb Show resolved Hide resolved
lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
spec/visual/samples/sas Outdated Show resolved Hide resolved
lib/rouge/lexers/sas.rb Show resolved Hide resolved
lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/sas.rb Show resolved Hide resolved
spec/visual/samples/sas 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 4, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jun 4, 2019

@tomsutch I do wonder if there's some shortcut to identifying keywords (such as by position) as it seems like there's a lot of matching being done to produce relatively few distinct tokens.

@tomsutch
Copy link
Contributor Author

tomsutch commented Jun 4, 2019

@pyrmont Thanks for your feedback. I'll take a look at your comments in detail over the coming days.

lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
lib/rouge/demos/sas Outdated Show resolved Hide resolved
lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
Pass string literals to mimetypes
Remove excess whitespace
Add link to CC-BY licence
Add examples of hex numbers to visual sample
* Use consistent naming for method and instance variable
* Use a Set for each list of keywords
* Wrap the proc_keywords Hash in a method
lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/sas.rb Outdated Show resolved Hide resolved
@ashmaroli
Copy link
Contributor

You'll need to resolve all offenses reported by RuboCop:
https://travis-ci.org/rouge-ruby/rouge/jobs/554691134

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 more corrections to regexps in the lexer..

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

pyrmont commented Jul 16, 2019

@tomsutch Any luck fixing the outstanding items in this PR?

tomsutch and others added 2 commits July 17, 2019 09:52
No need to re-split text as we are capturing terms separately
Use upper-case keys in proc_keywords for simplicity
Co-Authored-By: Ashwin Maroli <ashmaroli@users.noreply.github.com>
@ashmaroli
Copy link
Contributor

@tomsutch I hate to inform you but looks like Travis CI failed with some linting errors. Please fix.
Thanks.

@pyrmont
Copy link
Contributor

pyrmont commented Jul 17, 2019

@tomsutch Before pushing, it's a good idea to run the following:

rake && bundle exec rubucop bin lib spec tasks

If that runs without error then your code should pass Travis as well.

@tomsutch
Copy link
Contributor Author

You'll need to resolve all offenses reported by RuboCop:
https://travis-ci.org/rouge-ruby/rouge/jobs/554691134

Thanks - the latest build is https://travis-ci.org/rouge-ruby/rouge/jobs/559860704
It consists entirely of 'offenses' like this

Lint/AmbiguousRegexpLiteral: Ambiguous regexp literal. Parenthesize the method arguments if it's surely a regexp literal, or add a whitespace to the right of the / if it should be a division.

There's one offense for every instance of rule /.../

I'm certainly happy to do what's needed to fix this, but I don't understand why these are happening when other lexers contain similar rules in similar formats and don't get picked up by Rubocop. Do you know why this might be?

@ashmaroli
Copy link
Contributor

The offense is simply rule /.../. Other lexers may have it like rule %r/.../ or rule(/.../)

@pyrmont
Copy link
Contributor

pyrmont commented Jul 17, 2019

@tomsutch The explanation given by RuboCop is brief but, essentially, RuboCop flags the use of literal regular expressions of the form /.../ in method arguments as ambiguous because it's not clear whether the initial / represents the beginning of a regular expression or a method called /.

Other lexers avoid this warning by either using a sigil to mark the beginning of the regex (%r so that it becomes %r/.../) or wrapping the parameters inside parentheses.

@tomsutch
Copy link
Contributor Author

@tomsutch The explanation given by RuboCop is brief but, essentially, RuboCop flags the use of literal regular expressions of the form /.../ in method arguments as ambiguous because it's not clear whether the initial / represents the beginning of a regular expression or a method called /.

Other lexers avoid this warning by either using a sigil to mark the beginning of the regex (%r so that it becomes %r/.../) or wrapping the parameters inside parentheses.

Ah, thanks. Now I see that the the other lexers I was referring to when putting this together have also been changed accordingly via #1180. I'll fix this now.

@tomsutch
Copy link
Contributor Author

I think I've now addressed all the feedback, if you're happy with my response to your request @pyrmont?
Thanks everyone for your patience!

@pyrmont pyrmont merged commit 9a895d9 into rouge-ruby:master Jul 17, 2019
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jul 17, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jul 17, 2019

@tomsutch Thanks for the contribution to Rouge :) Another language that's highlighted! 🎉

@tomsutch tomsutch deleted the sas-lexer branch July 17, 2019 21:58
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

4 participants