atom / language-ruby

Ruby package for Atom
Other
101 stars 144 forks source link

Ruby code is not getting the right colors. #252

Open cristianstu opened 5 years ago

cristianstu commented 5 years ago

Description

Ruby code is not getting the right colors. Light theme seems to be ok.

Steps to Reproduce

  1. Upload to atom >= 1.32.0
  2. Open any ruby file.

Expected behavior: Show the file like this

image

Actual behavior:

image

Reproduces how often: Always

Versions

Atom : 1.32.1 Electron: 2.0.9 Chrome : 61.0.3163.100 Node : 8.9.3

apm 2.1.2 npm 6.2.0 node 8.9.3 x64 atom 1.32.1 python 2.7.15+ git 2.19.1

Additional Information

Seems like syntax--ruby class is missing on hierarchy. Switching to Ruby On Rails syntax fix the problem.

Arcanemagus commented 5 years ago

Atom v1.32.0 enabled a new parsing system by default. If you have specific issues about how the syntax highlighting has changed that you feel are mistakes, please take a look at the open issues on atom/language-ruby. If the specific problem that you're experiencing is not already being tracked there, please open a new issue on that repository.

If you think that this issue is related to this theme in particular could you outline what scopes aren't being colored?

Tree-sitter is a new grammar system that enables a number of advanced features including more rigorous syntax highlighting, which means more tokens will be detected correctly or more correctly than in the past. This means that colors may change because of this new information. What we're mostly looking for are situations where the underlying scopes of tokens are incorrect, not that the color is different than it was before. Some differences may be expected, such as variables now being recognized the same in every location they appear instead of differently based on the context. In general, we don't consider that a bug.

If you want to see how Tree-sitter identifies the token at the current cursor position, you can open the Command Palette using Cmd+Shift+P on macOS or Ctrl+Shift+P on other platforms, then search for and execute "Editor: Log Cursor Syntax Tree Scope". If the scope at the bottom is incorrect, please file an issue and describe what you expected instead.

Thanks!

cristianstu commented 5 years ago

@Arcanemagus I think everything being identified correctly but is not getting the proper colors. (checked with Log Cursor)

Class names, for instance. The color should be @yellow

https://github.com/atom/solarized-dark-syntax/blob/c4ed1ac41a5dd2120b912a766f4989e5b346bc33/styles/syntax/ruby.less#L37

is not being applied because it depends on parent container class syntax--ruby, but that class is missing in the hierarchy.

I guess adding that class is a language-ruby responsibility, or it was before, but if they have changed that class, this package will need to change that styles, right?

Thanks!

simurai commented 5 years ago

I can reproduce this as well. It seems that the example code of

class Article < ApplicationRecord
  belongs_to :category, optional: true
end

doesn't get wrapped with the syntax--source syntax--ruby classes on the parent <span>:

screen shot 2018-11-14 at 2 27 43 pm

Is that intentional? It means that all the styling in https://github.com/atom/solarized-dark-syntax/blob/c8a93df669e7a9a8ca062eb35426911bf864ed3b/styles/syntax/ruby.less#L1 doesn't get applied.

Versions

Atom    : 1.34.0-nightly12
Electron: 2.0.12
Chrome  : 61.0.3163.100
Node    : 8.9.3

I'll move this over to language-ruby.

Ben3eeE commented 5 years ago

@simurai The missing syntax--source syntax--ruby should be fixed in Atom 1.32.2 and later via https://github.com/atom/language-ruby/commit/0208636827aef4f420c5d94ec4ef43bbaf8a264e.

Which version of the package is nightly12 using?

Ben3eeE commented 5 years ago

Running a build from latest master looks like this with solarized-dark-syntax: image

simurai commented 5 years ago

should be fixed in Atom 1.32.2 and later

Nice! 🙌

Which version of the package is nightly12 using?

Ouch.. sorry, this is embarrassing. :sweat_smile: I had some other example code above

class Article < ApplicationRecord
  belongs_to :category, optional: true
end

... which then probably messed up the highlighting. OK, works also in nightly12. 👍 Closing! And thanks for the checking. @Ben3eeE

josephchoe commented 5 years ago

If some of the colors still don't look right on the solarized dark theme, but the syntax--ruby classes are being populated on the hierarchy, does that mean this issue needs to go back to the atom/solarized-dark-syntax repository?

For example:

screen shot 2018-11-19 at 2 18 35 am
simurai commented 5 years ago

@josephchoe does that mean this issue needs to go back to the atom/solarized-dark-syntax repository?

Yes, it seems that require gets intentionally overridden here: https://github.com/atom/solarized-dark-syntax/blob/74bc0df24ca7493d9d8bcfcaedccb5cc05c2c2f9/styles/syntax/ruby.less#L123-L125

Removing it, makes require blue. Not sure if that is prefered?

image

josephchoe commented 5 years ago

I think it's supposed to be like this? Though I might be remembering wrong.

screen shot 2018-11-22 at 9 57 07 pm
simurai commented 5 years ago

@josephchoe does that mean this issue needs to go back to the atom/solarized-dark-syntax repository? @josephchoe I think it's supposed to be like this?

Sorry, you're right. The scopes seem to be different:

Before (TextMate) After (Tree-sitter)
textmate tree-sitter
solarized-textmate solarized-tree-sitter

But it's different for all themes, not just Solarized. Not sure if it's possible to change the Tree-sitter grammar so that it matches TextMate's?

I'll re-open it here. It might be a duplicate of any of these https://github.com/atom/language-ruby/labels/tree-sitter.

maxbrunsfeld commented 5 years ago

I think it's supposed to be like this?

IMO the new behavior makes more sense. require isn't a keyword; it's a built-in function, so I think support.function is the right scope. If solarized doesn't highlight support.function, then I think a lot of languages are going to look wrong, not just Ruby, because that's the scope used for "special" functions in many languages.

gsmetal commented 5 years ago

Hello, any updates here? There are a lot of problems with highlighting on tree sitter. For example: image

  1. Aren't include and extend built-in functions same as require?
  2. Function's argument with assignment is not highlighted as argument.
  3. super is not highlighted at all.
  4. Last var is highlighted as function, but it's definitely not a function.
  5. ...

Edited: I am using atom 1.36.0, it's monokai syntax theme, but that problems on all themes including builtin.

Ben3eeE commented 5 years ago
  1. Aren't include and extend built-in functions same as require?

I don't know Ruby so I can't answer this question but if they are you can add them here and change it from exact to match so it's a regex. There are quite a few built in functions that are not scoped.

https://github.com/atom/language-ruby/blob/0592946a72481f307860927e8c5a37c705388985/grammars/tree-sitter-ruby.cson#L155

  1. Function's argument with assignment is not highlighted as argument.

This should be easy to fix by just adding: 'optional_parameter > identifier': 'variable.parameter.function'

Here: https://github.com/atom/language-ruby/blob/0592946a72481f307860927e8c5a37c705388985/grammars/tree-sitter-ruby.cson#L160

  1. super is not highlighted at all.

Same thing. Add a line for it 'super': 'the-scope.i-want' or '"super"': 'the-scope.i-want'. The one that works is the correct one.

  1. Last var is highlighted as function, but it's definitely not a function.

Not as easy to fix as the other things.

  1. ...

...

PRs are welcome to add these things.

chbk commented 3 years ago

I believe this can be closed when #291 is merged.