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

Lower-case-ing the heredoc language #554

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

mauricioszabo
Copy link
Contributor

Two small fixes here:

  1. Usually, on Ruby, "heredocs" are always uppercase. So I'm lower-casing so that it tries to get the right grammar
  2. When a grammar is not found, heredocs are highlighted as strings

@savetheclocktower
Copy link
Sponsor Contributor

Typically the injectionRegex has been written to capture either uppercase or lowercase, but I don't have a problem with this. Seems like a more elegant way of handling it.

@mauricioszabo
Copy link
Contributor Author

@savetheclocktower just wanted to be sure what do you think about the (heredoc_content) being highlighted as string, but if you don't have any problem with it, I'll just write a test case and be done with this :)

@savetheclocktower
Copy link
Sponsor Contributor

Doesn't heredoc_body take care of that?

@mauricioszabo
Copy link
Contributor Author

Not really, it was only highlighting around the doc, not the contents

@savetheclocktower
Copy link
Sponsor Contributor

So heredoc_content is a child of heredoc_body. We should choose to apply string.unquoted.ruby to one of them, not both, or else the scope will get repeated.

confused-Techie
confused-Techie previously approved these changes Jun 9, 2023
@savetheclocktower
Copy link
Sponsor Contributor

Hang on. This feedback still needs to be addressed:

So heredoc_content is a child of heredoc_body. We should choose to apply string.unquoted.ruby to one of them, not both, or else the scope will get repeated

Copy link
Sponsor Contributor

@savetheclocktower savetheclocktower left a comment

Choose a reason for hiding this comment

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

Need to apply scope to heredoc_content or heredoc_body, not both.

@confused-Techie confused-Techie dismissed their stale review June 9, 2023 05:56

Didn't realize further changes needed to be addressed

@mauricioszabo
Copy link
Contributor Author

Ok, @savetheclocktower, it seems that it's better to apply only to heredoc_body because heredoc_content only applies to the inside of the string - so the highlighting could become really weird...

@@ -100,8 +99,7 @@

; "Bar" in `Foo::Bar`.
(scope_resolution
name: (constant) @support.other.class.ruby
(#set! test.final "true"))
name: (constant) @support.other.class.ruby)
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

The reason I had for doing test.final here — and it's more of a feeling than anything — is that I felt we didn't really gain anything from tagging one range as both support.other.class.ruby and constant.ruby.

There are places where it makes sense to do stuff like that, but if we apply both scopes, we end up leaving it to the syntax theme to decide which styles win out, and often that's not an intentional decision by the theme author as much as it's just an accident of CSS specificity and ordering of selectors. It also brings ordering in the SCM file into play — since constant.ruby is declared later in the file, that scope name will be in the inner span, and that also has an effect on which styles win out.

Technically, lots of things are marked as constant in tree-sitter-ruby, just as lots of things are marked as identifier, and I'm not certain that we give the user much useful information by scoping each one as constant even when it serves a more specific function.

I've tended to assign a constant scope only for (a) number literals, (b) built-in language constants like booleans and null, and (c) declarations that LOOK_LIKE_CONSTANTS and don't serve other functions. Lots of TM-style themes go even more conservative than that — using constant for A and B but using the variable scope for C.

I'm not arguing that any of this is necessarily wrong, and I'm happy to see how it shakes out, but that was the thinking behind the original decision.

Comment on lines -266 to -275
(#match? @string.quoted.other.interpolated.ruby "^%Q")
(#set! test.final true))


(
(string
"\"" @punctuation.definition.string.begin.ruby
(string_content)?
"\"" @punctuation.definition.string.end.ruby)
@string.quoted.other.ruby
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Was this section not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were some misbehaviors on the string matching - some interpolated strings were being detected as non-interpolated, and vice-versa. When I fixed these, I extracted most of the changes into specific matches.

In this specific case, I did not remove the match of double-quoted strings - I just removed the restriction on the previous rule (https://github.com/pulsar-edit/pulsar/pull/554/files#diff-82574a2099439cbee59d5f974dbe593145214402078d948cf9f49378d9637e40R264-L266) and that basically became the exact same match that we have here :D

@Spiker985
Copy link
Member

Perhaps this PR can be updated since we've had quite a lot of Tree-Sitter related fixes implemented since this was originally opened?

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