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 lexer for ReasonML #1248

Merged
merged 3 commits into from Jul 16, 2019
Merged

Add lexer for ReasonML #1248

merged 3 commits into from Jul 16, 2019

Conversation

sazarkin
Copy link
Contributor

@sazarkin sazarkin commented Jul 3, 2019

Hi, I've added support for ReasonML https://reasonml.github.io/

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.

Awesome! A ReasonML lexer! Had a couple of questions :)

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

pyrmont commented Jul 3, 2019

@jneen Do you have an opinion on the way this has been structured? Rather than using the OCaml lexer as the superclass and simply overriding the bits that don't make sense, @sazarkin has separated the shared elements into a common class that both lexers inherit from (similar to how Rouge::Lexers::TypescriptCommon works).

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed maintainer-action The PR has been reviewed but action by a maintainer is required labels Jul 3, 2019
@jneen
Copy link
Member

jneen commented Jul 4, 2019

seems ok to me

@sazarkin
Copy link
Contributor Author

sazarkin commented Jul 4, 2019

Thanks for a great feedback, I'll try to improve this PR

@ashmaroli
Copy link
Contributor

@sazarkin IMO, the entire :comment state can be removed and just have the following at ln#27

rule %r(/\*.*?\*/)m, Comment

@jneen
Copy link
Member

jneen commented Jul 5, 2019

@sazarkin IMO, the entire :comment state can be removed and just have the following at ln#27

rule %r(/\*.*?\*/)m, Comment

@ashmaroli we actually encourage breaking lexers down into further states, as it makes it possible to embed or escape within those states.

(edit): also... it can't. These comments are nestable (see the invocation of push).

@jneen
Copy link
Member

jneen commented Jul 5, 2019

It could stand to be written a little more neatly though, with less backslashes:

rule %r|[^/*)]+|, Comment
rule %r|[/][*]|, Comment, :push
rule %r|[*][/]|, Comment, :pop!
rule %r|[*/]|, Comment

@pyrmont pyrmont removed the maintainer-action The PR has been reviewed but action by a maintainer is required label Jul 7, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jul 15, 2019

@sazarkin See also @jneen's suggestion above about how to express the comment rules.

@sazarkin
Copy link
Contributor Author

Updated the comment part

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

pyrmont commented Jul 16, 2019

@sazarkin Thanks! Great to have a lexer for ReasonML as part of Rouge 🎉

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