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

Highlight Ruby's and/or/not logical operators #1950

Merged
merged 2 commits into from
May 15, 2023

Conversation

epidemian
Copy link
Contributor

I was trying to write a thingy using Jekyll and the Rouge highlighter, and noticed that Ruby's "wordy" logical operators, like or, were not being highlighted like other keywords.

This PR adds the and, or and not keywords to the Ruby lexer (see ruby-lang keywords reference).

It also adds some statements showcasing these operators on the Ruby visual sample page, which i checked and seems to be being highlighted correctly.

If this is all that's needed for supporting these keywords, then kudos for such an extensible and approachable codebase; the changes were super obvious to make :)

@tancnle
Copy link
Collaborator

tancnle commented May 8, 2023

Thank you for your contribution @epidemian ❤️ I'd suggest using Operator::Word token for these logical operators instead of Keyword. That would also make this comparable with Pygments (see related code).

What do you think about the following tweaks?

❯ git diff
diff --git lib/rouge/lexers/ruby.rb lib/rouge/lexers/ruby.rb
index 98bcca4f..42fb7849 100644
--- lib/rouge/lexers/ruby.rb
+++ lib/rouge/lexers/ruby.rb
@@ -106,8 +106,8 @@ module Rouge
       end
 
       keywords = %w(
-        BEGIN END alias and begin break case defined\? do else elsif end
-        ensure for if in next not or redo rescue raise retry return super then
+        BEGIN END alias begin break case defined\? do else elsif end
+        ensure for if in next redo rescue raise retry return super then
         undef unless until when while yield
       )
 
@@ -188,6 +188,7 @@ module Rouge
 
         rule %r/(?:#{keywords.join('|')})(?=\W|$)/, Keyword, :expr_start
         rule %r/(?:#{keywords_pseudo.join('|')})\b/, Keyword::Pseudo, :expr_start
+        rule %r/(not|and|or)\b/, Operator::Word
 
         rule %r(
           (module)

@tancnle tancnle self-requested a review May 8, 2023 06:13
@tancnle tancnle added the author-action The PR has been reviewed but action by the author is needed label May 8, 2023
@epidemian
Copy link
Contributor Author

Thanks for the suggestions @tancnle! I had no idea about this Operator.Word tokens. Got two questions about them though.

The first one: isn't the :expr_start argument needed when calling rule with these word operators as it is on the other keywords?

And the second one: are we sure these should be highlighted as operators instead of as control flow keywords like if or unless?

Marking them as keywords, the highlighting on the sample file looks like this:

image

While highlighting them as Word.Operator looks like this:

image

Now, the highlighting as Word.Operator makes them look the same as other operators like == or +, which makes sense.

But at the same time, these operators (specifically and and or) are used more as control flow keywords in Ruby than as a logical operators to join boolean conditions together (as is the case in Python). See Avdi's post about their usage for a nice exploration on these.

For example, if i write:

payment.valid? or raise "Something wrong with your payment, sorry"

This is more similar to using an unless guard than the typical use of the || operator. It's equivalent to:

raise "Something wrong with your payment, sorry" unless payment.valid?

(With the added benefit using or makes reading things left-to-right follow the order of evaluation.)

And also note that, coincidentally, Github also highlights or and unless the same way.

So, to summarize, i think marking these operators as Word.Operator would make more sense in a language like Python, where they are the only local operators in fact. But in Ruby, i think marking them as keywords makes more sense, since in practice they are used more like if and unless than as && and ||.

This is by no means a hard preference on my part, and i'd be happy to go with either alternative. But i would like to know your opinion before applying those changes :)

@tancnle
Copy link
Collaborator

tancnle commented May 14, 2023

Thank you for expanding on your rationale, @epidemian 👍🏼

isn't the :expr_start argument needed when calling rule with these word operators as it is on the other keywords?

Yes, it is needed. Sorry it was an oversight on my part.

With expr_start Without expr_start
Screenshot 2023-05-14 at 9 40 26 am Screenshot 2023-05-14 at 9 39 52 am

are we sure these should be highlighted as operators instead of as control flow keywords like if or unless?

I would say they are control flow operators and hence we should use Operator::Word token.

@epidemian
Copy link
Contributor Author

@tancnle awesome, thanks for the heads-up about :expr_start!

I added that state and changed these operators to be marked as Operator.Word tokens as you suggested.

I would say they are control flow operators and hence we should use Operator::Word token.

No hard objections really. It is after all a more specific token than just marking these as Keyword. And at the end of the day it's a concern of the highlighting color theme whether to use the same colors as other keywords or not :)

@tancnle
Copy link
Collaborator

tancnle commented May 15, 2023

Great stuff @epidemian. Thank you for your collaboration and understanding. Really appreciated. Once the CI goes through, I will merge it 🚀

@tancnle tancnle added this pull request to the merge queue May 15, 2023
Merged via the queue into rouge-ruby:master with commit c745bb8 May 15, 2023
7 checks passed
@epidemian epidemian deleted the ruby-wordy-logic branch May 15, 2023 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author-action The PR has been reviewed but action by the author is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants