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 use of # character in Racket lexer #1472

Merged
merged 10 commits into from
Apr 2, 2020

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Mar 31, 2020

The # character can be used in Racket in multiline comments as well as in boolean constants. As described in #1471, the Racket lexer does not at present correctly lex multiline comments or full-word (i.e. #true as opposed to #t) boolean constants.

This PR fixes those bugs (and will close #1471). It also removes extraneous code from the visual sample.

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Mar 31, 2020
@pyrmont pyrmont self-assigned this Mar 31, 2020
@pyrmont
Copy link
Contributor Author

pyrmont commented Mar 31, 2020

How's this look @yugenekr?

spec/visual/samples/racket Outdated Show resolved Hide resolved
spec/visual/samples/racket Outdated Show resolved Hide resolved
@yugaego
Copy link

yugaego commented Mar 31, 2020

Hi @pyrmont, thank you for working on this.

The updates you provided look mostly fine to me (I'm not Ruby or Rouge person) in regard the particular use cases I provided.

If to dive into a bit more details, more use cases might need to be covered.

I believe Racket parser documentation provides the list of all # use cases.
For example, it mentions the uppercase #T and #F also should be treated as booleans. (I'm not aware if the matching rules are case-insensitive.)

Also, see several comments examples from this documentation page

This section might add some context to the Racket comments usage:
https://docs.racket-lang.org/style/Choosing_the_Right_Construct.html?q=comments#%28part._.Comments%29

Do you think it could be worthwhile working on the rest of the cases? Or keep it till the exact need arises?

@pyrmont
Copy link
Contributor Author

pyrmont commented Mar 31, 2020

@yugenekr I think having support for commenting out s-expressions would be cool (and I don't think too difficult). Will have to add another state but should be straightforward. I'll have a tinker.

@pyrmont
Copy link
Contributor Author

pyrmont commented Mar 31, 2020

@yugenekr Famous last words, I guess. I realised looking at the list of datums in Racket that it could actually get quite complicated. I've tried a relatively simple approach that I think will cover most cases. How does it look to you?

@yugaego
Copy link

yugaego commented Apr 1, 2020

I agree these changes should be sufficient for now.
Other cases may be covered when needed.
Thanks!

Copy link

@yugaego yugaego left a comment

Choose a reason for hiding this comment

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

Minor notice. I'm not aware of exact usages of these samples. And from my perspective it might be more descriptive to use doc comments such as: ;; Highlight booleans, (...) should be highlighted as the commented text or similar.

@pyrmont pyrmont merged commit c4bd736 into rouge-ruby:master Apr 2, 2020
@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label Apr 2, 2020
@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 2, 2020

@yugenekr Thanks for your help on this one. The update version will go out as part of the next release of Rouge. That's scheduled for Tuesday 14 April 🎉

@pyrmont pyrmont deleted the bugfix.racket-comments branch April 2, 2020 10:11
@yugaego
Copy link

yugaego commented Apr 2, 2020

Great! Thanks a lot reacting fast with the updates.

mattt pushed a commit to NSHipster/rouge that referenced this pull request May 21, 2020
The # character can be used in Racket in multiline comments as well as
in boolean constants. Currently, the Racket lexer does not at correctly
lex multiline comments, expression comments or full-word boolean
constants.

This commit adds support for those constructs. It also removes
extraneous code from the visual sample.
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.

Racket number sign (hashtag) highlighting
2 participants