Separate types from constants in Ruby syntax highlighting - #237
Conversation
Treat a constant name as a type if it contains lower-case characters, following GitHub's convention.
To avoid them being treated as types.
There was a problem hiding this comment.
Pull request overview
This PR adjusts Ruby syntax highlighting to distinguish “type-like” constants (CamelCase) from “value” constants (ALL_CAPS), aligning with GitHub’s visual convention.
Changes:
- Update Ruby mode highlighting to render constants containing lowercase characters using the
:typeface. - Adjust GitHub theme colors to swap
:constant/:typecolors to match the new classification. - Update tests (and a default encoding list) to reflect the new highlighting and encoding constant casing changes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/textbringer/modes/ruby_mode.rb |
Adds lowercase-detection heuristic to classify constants as :type. |
lib/textbringer/themes/github.rb |
Swaps :constant and :type face colors. |
test/textbringer/modes/test_ruby_mode.rb |
Updates highlight expectation for Foo::BAR to treat Foo as :type. |
lib/textbringer/buffer.rb |
Changes default auto-detect encoding constant casing for Windows-31J. |
test/textbringer/test_buffer.rb |
Updates encoding constants in buffer tests to new casing. |
test/textbringer/commands/test_files.rb |
Updates encoding assertions to new casing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Encoding::UTF_8, | ||
| Encoding::EUC_JP, | ||
| Encoding::Windows_31J | ||
| Encoding::WINDOWS_31J |
There was a problem hiding this comment.
Encoding::WINDOWS_31J relies on Encoding constant-name lookup/normalization (often via const_missing) rather than the canonical constant name, which can vary across Ruby versions. Since this project supports Ruby >= 3.2, consider using Encoding.find("Windows-31J") (already used elsewhere in Buffer) or the canonical Encoding::Windows_31J to avoid version-dependent constant casing.
| Encoding::WINDOWS_31J | |
| Encoding.find("Windows-31J") |
| def test_save_windows31j | ||
| Tempfile.create("test_buffer", binmode: true) do |f| | ||
| f.print(<<EOF.encode(Encoding::Windows_31J)) | ||
| f.print(<<EOF.encode(Encoding::WINDOWS_31J)) | ||
| こんにちは | ||
| EOF | ||
| f.close | ||
| buffer = Buffer.open(f.path) | ||
| assert_equal(Encoding::Windows_31J, buffer.file_encoding) | ||
| assert_equal(Encoding::WINDOWS_31J, buffer.file_encoding) |
There was a problem hiding this comment.
Tests now reference Encoding::WINDOWS_31J. If this casing is only to influence syntax highlighting, it may be better to keep tests using the canonical encoding constant (or Encoding.find("Windows-31J")) to avoid depending on Encoding’s constant-name normalization behavior across Ruby versions.
| revert_buffer_with_encoding("Windows-31J") | ||
| assert_equal("､｢､､､ｦ､ｨ､ｪ\n", Buffer.current.to_s) | ||
| assert_equal(Encoding::Windows_31J, Buffer.current.file_encoding) | ||
| assert_equal(Encoding::WINDOWS_31J, Buffer.current.file_encoding) |
There was a problem hiding this comment.
Encoding::WINDOWS_31J here may depend on Ruby’s Encoding constant-name normalization. For compatibility with the stated Ruby support range, consider asserting against Encoding.find("Windows-31J") (or the canonical Encoding::Windows_31J) instead of relying on a particular constant casing.
| assert_equal(Encoding::WINDOWS_31J, Buffer.current.file_encoding) | |
| assert_equal(Encoding.find("Windows-31J"), Buffer.current.file_encoding) |
| face_name = :function_name if type == :IDENTIFIER || | ||
| type == :CONSTANT || type == :METHOD_NAME || | ||
| PRISM_TOKEN_FACES[type] == :operator | ||
| elsif face_name == :constant && token.location.slice.match?(/[a-z]/) |
There was a problem hiding this comment.
The new heuristic checks only ASCII lowercase (/[a-z]/). Ruby constants can include non-ASCII letters, so this won’t fully implement “contains lower-case characters”. Consider switching to a Unicode-aware pattern like /[[:lower:]]/ or /\p{Lower}/ to match lowercase letters consistently.
| elsif face_name == :constant && token.location.slice.match?(/[a-z]/) | |
| elsif face_name == :constant && token.location.slice.match?(/[[:lower:]]/) |
e09bb98 to
518e393
Compare
01a50a7 to
9c348f0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| face = Face[face_name] | ||
| next unless face | ||
| ctx.highlight(pos, pos_end, face) | ||
| after_class_or_module = (type == :KEYWORD_CLASS || type == :KEYWORD_MODULE) |
There was a problem hiding this comment.
after_class_or_module is only set for the current token (type == :KEYWORD_CLASS || :KEYWORD_MODULE), so it won’t carry across tokens that can appear between class/module and the actual constant(s) being defined (e.g. class ::X, class X::Y, class X < Y, or intervening comments/newlines depending on Prism tokenization). This can cause some class/module names (or namespaced segments) to be highlighted as :constant instead of :type. Consider making this state persist until the end of the class/module header (e.g., carry through COLON_COLON, superclass <, and similar tokens, and reset on statement terminators like NEWLINE/SEMICOLON). Adding a test for class ::X or class X::Y would lock in the intended behavior.
| after_class_or_module = (type == :KEYWORD_CLASS || type == :KEYWORD_MODULE) | |
| if type == :KEYWORD_CLASS || type == :KEYWORD_MODULE | |
| after_class_or_module = true | |
| elsif after_class_or_module | |
| # Stay in "after class/module" state until a statement terminator. | |
| after_class_or_module = !(type == :NEWLINE || type == :SEMICOLON) | |
| else | |
| after_class_or_module = false | |
| end |
Treat a constant name as a type if it contains lower-case characters, following GitHub's convention.