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

Fix comment parsing in Console lexer #1379

Merged
merged 4 commits into from Dec 23, 2019

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Dec 17, 2019

The Console lexer would not correctly parse comments if only the comments option was enabled. This was because of a bug in the way the prompt regular expression pattern. This PR fixes that bug and adds a series of tests to confirm the behaviour of the lexer is as intended.

This fixes #1370.

@pyrmont
Copy link
Contributor Author

pyrmont commented Dec 17, 2019

@dleidert Progress was delayed by a few things but I think I finally found the bug. I've patched that in this PR and added some tests to confirm this. Can you see how it looks on your end?

@pyrmont pyrmont self-assigned this Dec 17, 2019
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Dec 17, 2019
@jneen
Copy link
Member

jneen commented Dec 17, 2019

Good catch!

@pyrmont
Copy link
Contributor Author

pyrmont commented Dec 18, 2019

@jneen Always feels good to squash a bug! :)

@dleidert
Copy link

dleidert commented Dec 18, 2019

I can confirm that using e.g. console?comments=1 works. BTW all these produce the same result:

console?comments
console?comments=1
console?comments=true
console?comments=false

Only console?comments=0 restores the same behavior as console?prompt=#. If both options are used, prompt seems to override the comments option independent of the order.

@pyrmont
Copy link
Contributor Author

pyrmont commented Dec 19, 2019

@dleidert Hmmm.... that comments=false result seems wrong, doesn't it? I think I can imagine why it's happening but it's not what the user would expect. I might file that as a separate bug and look into fixing it.

As for the order not mattering, that's the behaviour working as intended. The way the Console lexer works is that it will first¹ attempt to match each line against the @prompt_regex regex pattern. When the prompt is set to #, that pattern is /[^<#]*?#/. Since the prefix ([^<#]*?)can match zero or more times, for any line that begins with a #, the prefix will match zero times and the ending character (#) will match once and it'll be treated as a prompt.

¹ As you can see when you follow the link, what it actually checks first is whether there's an elision (as now described in this code comment).

I've made an additional commit that adds some documentation to the beginning of the class that hopefully explains things a bit better. How does it look?

@dleidert
Copy link

@dleidert Hmmm.... that comments=false result seems wrong, doesn't it?

That's what I thought.

As for the order not mattering, that's the behaviour working as intended. [..]

Ok. Makes sense. This is probably more a matter of documenting the behavior.

I've made an additional commit that adds some documentation to the beginning of the class that hopefully explains things a bit better. How does it look?

Looks good to me :) Would be nice to have something like this at https://github.com/rouge-ruby/rouge/wiki/List-of-supported-languages-and-lexers or a sub-site for the console lexer. I can add it myself if you want me.

@pyrmont
Copy link
Contributor Author

pyrmont commented Dec 23, 2019

@dleidert wrote:

Would be nice to have something like this at https://github.com/rouge-ruby/rouge/wiki/List-of-supported-languages-and-lexers or a sub-site for the console lexer. I can add it myself if you want me.

I think adding it to the wiki is probably best for now. Speaking personally (and not for the other maintainers), I would like to have all documentation be centralised at rouge-ruby.github.io/docs/ and trash the wiki but even if we eventually decide to move in that direction, it would be a large scale effort and, in the meantime, having up to date information on the wiki seems like the smart move.

I should note that, in case it wasn't obvious, the docs for the ConsoleLexer class will be updated but that won't happen until with the next release of Rouge on Tuesday 14 January.

@pyrmont pyrmont merged commit bf40c4c into rouge-ruby:master Dec 23, 2019
@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label Dec 23, 2019
@pyrmont pyrmont deleted the bugfix.console-lexer-options branch April 3, 2020 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.

Please document usage of console options (e.g. ?prompt, ?root, etc.)
3 participants