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

Improve robustness of Crystal lexer #1644

Merged
merged 3 commits into from
Dec 8, 2020

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Dec 5, 2020

Crystal's visual sample was not very comprehensive and meant that the lexer did not cover elements of the syntax (notably macros and % conditionals).

This PR adds a more comprehensive visual sample (based on the one in Pygments) and makes changes to the lexer to fix coverage errors.

@pyrmont
Copy link
Contributor Author

pyrmont commented Dec 5, 2020

@rymiel I'm not sure if you're in a position to do so but if you could have a peek at this, it'd be much appreciated. I've added a more comprehensive visual sample for the Crystal lexer and tried to fix some of the areas in which the lexer was deficient.

Copy link
Contributor

@rymiel rymiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in no way in any authoritative position on this, I simply was annoyed that half of my files were turning bland, though, I read through it from a simple user's perspective, I can't immediately think of any language constructs missing, though I'll hopefully continue to participate on this topic.

spec/visual/samples/crystal Outdated Show resolved Hide resolved
@pyrmont
Copy link
Contributor Author

pyrmont commented Dec 7, 2020

@rymiel Cool. Well, I think I'll merge it in sometime tomorrow and if there are any issues, I'm sure they'll be reported soon enough. Thanks for taking a look!

@rymiel
Copy link
Contributor

rymiel commented Dec 7, 2020

@pyrmont I tried really hard to break it but the lexer is very robust as of now. Syntax that breaks other lexers like the one used in github doesn't break rouge, however, after trying really hard and going back to what broke it before, regex literals, I've found that nested regex interpolation /#{/#{nested}/}/ does not work correctly. Yes, it's ridiculous, but it compiles. Note that whatever the crystal playground uses is less robust than rouge

I haven't investigated the cause yet but mostly I just wanted to say how much I struggled to find any problems.

@pyrmont
Copy link
Contributor Author

pyrmont commented Dec 8, 2020

@rymiel That's great to hear. I can't really take any credit, the Crystal lexer is very closely based on Rouge's Ruby lexer and I think it's the best around (probably not a surprise for a Ruby lexing library). I'll probably leave the nested regular expressions for now and see if it comes up naturally as an issue.

Thanks for giving it such a thorough test! It was really helpful :)

@pyrmont pyrmont merged commit 926f864 into rouge-ruby:master Dec 8, 2020
@pyrmont pyrmont deleted the bugfix.more-robust-crystal-lexer branch December 8, 2020 01:05
mattt pushed a commit to NSHipster/rouge that referenced this pull request May 19, 2021
…#1644)

The Crystal lexer does not include a very extensive visual sample. This
commit adds a sample based on the one included in the Pygments lexer. As
a result of testing with the sample, issues were identified with the
lexing of macros and the `{%` and `%}` conditionals. These were fixed.
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

2 participants