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 JSL Lexer #871

Merged
merged 17 commits into from
Aug 31, 2019
Merged

Add JSL Lexer #871

merged 17 commits into from
Aug 31, 2019

Conversation

justinc11
Copy link
Contributor

This PR adds support for JSL, the scripting language used in JMP software.

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, thanks for your submission. I'm trying to help triage pull requests. Just one minor nit, which I guess is down to the age of your pull request..

lib/rouge/lexers/jsl.rb Show resolved Hide resolved
spec/lexers/jsl_spec.rb Show resolved Hide resolved
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Aug 4, 2019
@pyrmont pyrmont self-assigned this Aug 26, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Aug 30, 2019

@justinc11 I'm sorry it's taken so long to get back to you on this one. I also apologise for force pushing to your branch: I had a few comments on your code but it's been so long since you submitted it that I felt bad asking you to make changes to something you might not remember that well any more.

Hopefully all the changes are pretty straightforward. I've tried to make the lexing rules a little more robust based on a cursory reading of the JSL documentation. Please let me know if anything stands out as broken.

@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 Aug 30, 2019
@justinc11
Copy link
Contributor Author

Hi @pyrmont, thanks a bunch for cleaning this up for me (it needed some help). Since I initially opened this pull request I realized I forgot to account for date literals (shown here), but didn't get to it until your response reminded me. I also found out about a name-literal syntax similar to SAS' name literals (e.g. "some name!"n) where you can have any characters in the name.

I pushed to a separate branch, date-name-literal, rules for the date and name literals (I didn't want to mess with your already reviewed code). Could you let me know if these look okay or if any changes are needed before merging into this pull request?

@pyrmont
Copy link
Contributor

pyrmont commented Aug 31, 2019

@justinc11 Yep, that branch looks generally OK. Can you merge it in?

@justinc11
Copy link
Contributor Author

Done. Thanks!

@pyrmont pyrmont merged commit 8168673 into rouge-ruby:master Aug 31, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Aug 31, 2019

@justinc11 Thanks for the submission! Sorry again that it took so long but we got there in the end. The next release of Rouge is scheduled for this coming Tuesday so this lexer will be included in version 3.10.0 🎉

@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Aug 31, 2019
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.

3 participants