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.

Sign up for GitHub

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

Jump to bottom

New lexer: Lutin #1307

Merged
merged 30 commits into from
Sep 3, 2019
Merged

New lexer: Lutin #1307

merged 30 commits into from
Sep 3, 2019

Conversation

jahierwan
Copy link
Contributor

jahierwan and others added 17 commits April 26, 2018 17:06
@jahierwan
New lexer: lustre
10ce91f
@jahierwan
New lexer: lustre (commiting the good files)
31b6f45
@jahierwan
New lexer: lutin
923d69b
Shorten the Lutin example.
7663fc5
@jahierwan @pyrmont
New lexer: lustre
cc5c91c
@jahierwan @pyrmont
New lexer: lustre (commiting the good files)
ce9836b
@jahierwan @pyrmont
New lexer: lutin
8eaae46
@pyrmont
Shorten the Lutin example.
837ed67
@jahierwan
Merge branch 'master' of /~https://github.com/jahierwan/rouge
a440f40
@jahierwan
Remove lutin support; I will submit them in a different PR as request…
c26b24b 
…ed by @pyrmont
@jahierwan
Cut down the demos/lustre example to a 5 lines program (#discussion_r…
b1c19c7 
…310300155)
@jahierwan
true and false should be Keyword::Constant, not Name::Builtin::Pseudo…
4228d11 
… (#discussion_r310299887)
@jahierwan
Don't worry about end of line in comments (#discussion_r310300155)
973aa8b
@jahierwan
Remove useless rules (#discussion_r310300940, #discussion_r310302349)
942d79e
@jahierwan
Remove support for Hex and octal number (as they are not supported in…
7bec098 
… the language
@jahierwan
fix an error (973aa8b) and remove warnings
d130378
@jahierwan
New lexer: Lutin
7c62be9
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Aug 19, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Aug 19, 2019

@jahierwan This still has the Lustre lexer files.

@jahierwan jahierwan mentioned this pull request Aug 19, 2019
Merged
@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 19, 2019
@jahierwan
Copy link
Contributor Author

@jahierwan This still has the Lustre lexer files.

oh, yes, I've forked my master copy which is under review, sorry.
Is it ok if I wait #905 PR to be accepted and merged, so that I can merge this branch against master
before pushing this branch? Or do you want me start from a fresh branch?

@pyrmont
Copy link
Contributor

pyrmont commented Aug 20, 2019 •
edited
Loading

You won't need to merge it back into your master. Once it's accepted, it'll be squashed and merged into the official master. You'll just merge the official master into your master to bring yourself up to date.

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

ok, thanks.

@pyrmont pyrmont self-assigned this Aug 25, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Aug 26, 2019

@jahierwan I'm sorry—my git foo wasn't strong enough to work out how to unpick this branch so I basically nuked it and started over. There's now just the current commit with the files as they were after your most recent commit. I'll have a look and get back to you with any comments ASAP.

pyrmont
pyrmont suggested changes Sep 1, 2019
View reviewed changes
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.

The Lutin lexer seems (at least superficially) almost identical to the Lustre lexer. You can use a lexer as the parent of another lexer and then simplify modify the parts that have changed (see the C++ lexer for an example).

Are these languages related enough that changes to one will require changes to the other? Is one a superset of the other for example?

lib/rouge/lexers/lutin.rb Show resolved Hide resolved
lib/rouge/lexers/lutin.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/lutin.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 Sep 1, 2019
@jahierwan
Copy link
Contributor Author

jahierwan commented Sep 3, 2019 •
edited
Loading

The Lutin lexer seems (at least superficially) almost identical to the Lustre lexer. You can use a lexer as the parent of another lexer and then simplify modify the parts that have changed (see the C++ lexer for an example).

Are these languages related enough that changes to one will require changes to the other? Is one a superset of the other for example?

Clearly the syntax of Lutin tried to mimick as much as possible to the one of Lustre. Nevertheless, they are independent languages and modifying one does not imply some modification to the other.

But I agree it would make sense to factorize the code.

@pyrmont pyrmont merged commit fd82b9c into rouge-ruby:master Sep 3, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Sep 3, 2019

@jahierwan Thanks for working through this one. I thought it would be good to have the Lustre and Lutin lexers ready for the same release (I'm about to push that out now). Hope you find it useful!

@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Sep 3, 2019
@jahierwan
Copy link
Contributor Author

jahierwan commented Sep 3, 2019 via email •
edited
Loading

@jahierwan jahierwan deleted the add-lutin branch September 4, 2019 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pyrmont pyrmont pyrmont approved these changes

Assignees

@pyrmont pyrmont

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jahierwan @pyrmont