Repository navigation
Fix #209: Syntax Highlighting for GitHub - #283
malay-satapathy wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a TextMate grammar for the Emboss language, complete with a README, Bazel build configuration, and a basic validation test. Feedback on the grammar file highlights several discrepancies with the official Emboss compiler specification: string patterns should be prioritized over comments to avoid incorrect highlighting inside string literals; unsupported keywords, builtins, and operators (such as union, $reserved, this, and /) should be removed; and regexes for numbers and constants should be refined to strictly match the compiler's tokenization rules. Additionally, it is recommended to expand the test suite to programmatically verify that all #include targets exist in the repository.
| { | ||
| "include": "#comments" | ||
| }, | ||
| { | ||
| "include": "#doc-comments" | ||
| }, | ||
| { | ||
| "include": "#strings" | ||
| }, |
There was a problem hiding this comment.
In the patterns array, #comments and #doc-comments are included before #strings. This causes any # or -- character inside a double-quoted string literal to be incorrectly highlighted as a comment. Moving #strings to the top of the patterns list ensures that string literals are matched first.
| { | |
| "include": "#comments" | |
| }, | |
| { | |
| "include": "#doc-comments" | |
| }, | |
| { | |
| "include": "#strings" | |
| }, | |
| { | |
| "include": "#strings" | |
| }, | |
| { | |
| "include": "#comments" | |
| }, | |
| { | |
| "include": "#doc-comments" | |
| }, |
| "name": "keyword.control.emboss", | ||
| "match": "\\b(struct|union|enum|bits|external|import|as|if|let)\\b" | ||
| }, | ||
| { |
There was a problem hiding this comment.
The keyword union is included in the control keywords list. However, Emboss does not support union (only struct, bits, and enum are valid container types). Including it here will incorrectly highlight union as a keyword when it is actually treated as a regular identifier (or SnakeWord) by the compiler.
{
"name": "keyword.control.emboss",
"match": "\\b(struct|enum|bits|external|import|as|if|let)\\b"
},| "name": "variable.language.emboss", | ||
| "match": "(?<!\\w)(\\$reserved|\\$default|\\$static_size_in_bits|\\$is_statically_sized|\\bthis|\\$next|\\$max|\\$present|\\$upper_bound|\\$lower_bound)\\b" | ||
| }, | ||
| { |
There was a problem hiding this comment.
The builtins list includes $reserved and this. Neither of these are valid builtins or keywords in the Emboss language. Any word starting with $ that is not explicitly in the compiler's LITERAL_TOKEN_PATTERNS (like $reserved) is tokenized as a BadWord and rejected. Similarly, this is treated as a regular identifier. They should be removed from the builtins list.
{
"name": "variable.language.emboss",
"match": "(?<!\\w)(\\$default|\\$static_size_in_bits|\\$is_statically_sized|\\$next|\\$max|\\$present|\\$upper_bound|\\$lower_bound)\\b"
},| "name": "constant.numeric.hex.emboss", | ||
| "match": "\\b0[xX][0-9a-fA-F]+(_[0-9a-fA-F]+)*\\b" | ||
| }, | ||
| { | ||
| "name": "constant.numeric.binary.emboss", | ||
| "match": "\\b0[bB][01]+(_[01]+)*\\b" | ||
| }, | ||
| { | ||
| "name": "constant.numeric.decimal.emboss", | ||
| "match": "\\b\\d+(_\\d+)*\\b" | ||
| } |
There was a problem hiding this comment.
The number regexes do not fully align with the Emboss compiler's tokenizer:
- Hex and binary numbers can have an optional underscore right after the prefix (e.g.,
0x_1234,0b_0101), which the current regexes do not support. - Decimal numbers only allow underscores when grouping digits in threes (e.g.,
1_000_000). The current regex\\b\\d+(_\\d+)*\\bincorrectly matches invalid formats like12_34(which the compiler rejects asBadNumber).
Updating these regexes ensures accurate syntax highlighting that matches the compiler's behavior.
| "name": "constant.numeric.hex.emboss", | |
| "match": "\\b0[xX][0-9a-fA-F]+(_[0-9a-fA-F]+)*\\b" | |
| }, | |
| { | |
| "name": "constant.numeric.binary.emboss", | |
| "match": "\\b0[bB][01]+(_[01]+)*\\b" | |
| }, | |
| { | |
| "name": "constant.numeric.decimal.emboss", | |
| "match": "\\b\\d+(_\\d+)*\\b" | |
| } | |
| "name": "constant.numeric.hex.emboss", | |
| "match": "\\b0[xX](?:[0-9a-fA-F]+|_?[0-9a-fA-F]{1,4}(?:_[0-9a-fA-F]{4})*|_?[0-9a-fA-F]{1,8}(?:_[0-9a-fA-F]{8})*)\\b" | |
| }, | |
| { | |
| "name": "constant.numeric.binary.emboss", | |
| "match": "\\b0[bB](?:[01]+|_?[01]{1,4}(?:_[01]{4})*|_?[01]{1,8}(?:_[01]{8})*)\\b" | |
| }, | |
| { | |
| "name": "constant.numeric.decimal.emboss", | |
| "match": "\\b(?:\\d+|\\d{1,3}(?:_\\d{3})+)\\b" |
| "patterns": [ | ||
| { | ||
| "name": "variable.other.constant.emboss", | ||
| "match": "\\b[A-Z][A-Z0-9_]+\\b" |
There was a problem hiding this comment.
The regex for constants (\\b[A-Z][A-Z0-9_]+\\b) matches single-letter-followed-by-number words like A1 or A123. However, the Emboss compiler explicitly disallows these as ShoutyWords (constants) to avoid ambiguity with CamelWords, treating them as BadWords. The regex should be updated to \\b[A-Z][A-Z_0-9]*[A-Z_][A-Z_0-9]*\\b to match the compiler's strict definition.
| "match": "\\b[A-Z][A-Z0-9_]+\\b" | |
| "match": "\\b[A-Z][A-Z_0-9]*[A-Z_][A-Z_0-9]*\\b" |
| "name": "keyword.operator.emboss", | ||
| "match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*|/" | ||
| } |
There was a problem hiding this comment.
The operators pattern includes / as an operator. However, Emboss does not support division, and / is not a valid token in the Emboss language. It should be removed.
| "name": "keyword.operator.emboss", | |
| "match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*|/" | |
| } | |
| "name": "keyword.operator.emboss", | |
| "match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*" | |
| } |
| self.assertEqual(data.get("name"), "Emboss") | ||
| self.assertEqual(data.get("scopeName"), "source.emboss") | ||
| self.assertIn("emb", data.get("fileTypes", [])) |
There was a problem hiding this comment.
We can make the test suite more robust by adding a test case that parses the entire emboss.tmLanguage.json file and verifies that all #include targets referenced in the patterns actually exist in the repository. This prevents broken references when the grammar is updated in the future.
self.assertEqual(data.get("name"), "Emboss")
self.assertEqual(data.get("scopeName"), "source.emboss")
self.assertIn("emb", data.get("fileTypes", []))
def test_all_includes_exist(self):
json_path = os.path.join(os.path.dirname(__file__), "emboss.tmLanguage.json")
with open(json_path) as f:
data = json.load(f)
repository = data.get("repository", {})
def check_patterns(patterns):
for pattern in patterns:
if "include" in pattern:
include = pattern["include"]
if include.startswith("#"):
self.assertIn(include[1:], repository)
if "patterns" in pattern:
check_patterns(pattern["patterns"])
if "patterns" in data:
check_patterns(data["patterns"])
for key, val in repository.items():
if "patterns" in val:
check_patterns(val["patterns"])
e50db34 to
dcfb0aa
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a TextMate grammar for the Emboss language, along with a Python test suite to validate the grammar's JSON structure and compile its regular expressions. The feedback suggests aligning the documentation comment scope with TextMate conventions by using a line comment scope instead of a block comment scope, and optimizing the test suite by parsing the JSON file once in setUpClass while making the pattern validation more robust.
| "doc-comments": { | ||
| "patterns": [ | ||
| { | ||
| "name": "comment.block.documentation.emboss", |
There was a problem hiding this comment.
According to TextMate naming conventions, single-line comments (like those starting with --) should be scoped under comment.line rather than comment.block. Using comment.line ensures that editors and themes correctly apply line-comment styles and behavior (such as comment toggling).
| "name": "comment.block.documentation.emboss", | |
| "name": "comment.line.double-dash.documentation.emboss", |
| class TextmateTest(unittest.TestCase): | ||
| def test_valid_json(self): | ||
| json_path = os.path.join(os.path.dirname(__file__), "emboss.tmLanguage.json") | ||
| with open(json_path) as f: | ||
| data = json.load(f) | ||
|
|
||
| self.assertEqual(data.get("name"), "Emboss") | ||
| self.assertEqual(data.get("scopeName"), "source.emboss") | ||
| self.assertIn("emb", data.get("fileTypes", [])) | ||
|
|
||
| def test_all_includes_exist_and_regexes_compile(self): | ||
| json_path = os.path.join(os.path.dirname(__file__), "emboss.tmLanguage.json") | ||
| with open(json_path) as f: | ||
| data = json.load(f) | ||
|
|
||
| repository = data.get("repository", {}) | ||
|
|
||
| def check_patterns(patterns): | ||
| for pattern in patterns: | ||
| if "include" in pattern: | ||
| include = pattern["include"] | ||
| if include.startswith("#"): | ||
| self.assertIn(include[1:], repository) | ||
| for regex_key in ("match", "begin", "end"): | ||
| if regex_key in pattern: | ||
| re.compile(pattern[regex_key]) | ||
| if "patterns" in pattern: | ||
| check_patterns(pattern["patterns"]) | ||
|
|
||
| if "patterns" in data: | ||
| check_patterns(data["patterns"]) | ||
| for val in repository.values(): | ||
| if "patterns" in val: | ||
| check_patterns(val["patterns"]) |
There was a problem hiding this comment.
We can optimize the test by loading and parsing the JSON file once in setUpClass instead of repeating it in every test method. Additionally, we can make the pattern validation more robust by recursively checking repository items directly as patterns, rather than assuming they always contain a nested patterns list.
class TextmateTest(unittest.TestCase):
@classmethod
def setUpClass(cls):
json_path = os.path.join(os.path.dirname(__file__), "emboss.tmLanguage.json")
with open(json_path) as f:
cls.data = json.load(f)
def test_valid_json(self):
self.assertEqual(self.data.get("name"), "Emboss")
self.assertEqual(self.data.get("scopeName"), "source.emboss")
self.assertIn("emb", self.data.get("fileTypes", []))
def test_all_includes_exist_and_regexes_compile(self):
repository = self.data.get("repository", {})
def check_pattern(pattern):
if "include" in pattern:
include = pattern["include"]
if include.startswith("#"):
self.assertIn(include[1:], repository)
for regex_key in ("match", "begin", "end"):
if regex_key in pattern:
re.compile(pattern[regex_key])
if "patterns" in pattern:
for p in pattern["patterns"]:
check_pattern(p)
if "patterns" in self.data:
for p in self.data["patterns"]:
check_pattern(p)
for val in repository.values():
check_pattern(val)
dcfb0aa to
3b8d334
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a TextMate grammar for the Emboss language, along with a README, a Bazel BUILD file, and a Python test suite to validate the grammar's JSON structure and regex compilation. The feedback suggests adding the assignment operator '=' to the operators pattern, ensuring a trailing newline in the JSON file, and explicitly specifying 'encoding="utf-8"' when opening files in Python.
| "patterns": [ | ||
| { | ||
| "name": "keyword.operator.emboss", | ||
| "match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*" |
There was a problem hiding this comment.
The assignment operator = is currently not matched by the operators pattern. Since = is a fundamental operator in Emboss (used for field and constant definitions), it should be highlighted. Adding it to the end of the alternation ensures that longer operators like == are still matched correctly first.
| "match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*" | |
| "match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*|=" |
| ] | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The JSON file is missing a trailing newline. It is a standard POSIX and Git best practice for all text files to end with a newline to ensure compatibility with various Unix tools and to avoid Git warnings.
| } | |
| } | |
| def setUpClass(cls): | ||
| super().setUpClass() | ||
| json_path = os.path.join(os.path.dirname(__file__), "emboss.tmLanguage.json") | ||
| with open(json_path) as f: |
There was a problem hiding this comment.
When opening text files in Python 3, it is highly recommended to specify an explicit encoding (such as encoding="utf-8"). This prevents platform-dependent encoding issues when the tests are run on environments with different default system encodings (e.g., Windows).
| with open(json_path) as f: | |
| with open(json_path, encoding="utf-8") as f: |
|
/gemini review |
3b8d334 to
806d95f
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a TextMate grammar definition (emboss.tmLanguage.json) for the Emboss language, accompanied by a README, a Bazel build file, and a Python test suite (textmate_test.py) to validate the grammar structure and ensure its regular expressions compile successfully. The feedback suggests enhancing the grammar by defining and including a new #punctuation pattern to highlight separators, brackets, parentheses, and accessors, which will improve the visual structure of Emboss code in editors.
| }, | ||
| { | ||
| "include": "#operators" | ||
| } |
There was a problem hiding this comment.
To ensure that the new punctuation patterns are applied during syntax highlighting, we should include the #punctuation pattern in the main patterns array.
{
"include": "#operators"
},
{
"include": "#punctuation"
}
],| "operators": { | ||
| "patterns": [ | ||
| { | ||
| "name": "keyword.operator.emboss", | ||
| "match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*|=" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Adding explicit highlighting for punctuation (such as colons, commas, brackets, parentheses, and member access dots) will significantly improve the readability and visual structure of Emboss code in editors like VS Code and GitHub.
"operators": {
"patterns": [
{
"name": "keyword.operator.emboss",
"match": "==|!=|<=|>=|<|>|&&|\\|\\||\\+|-|\\*|="
}
]
},
"punctuation": {
"patterns": [
{
"name": "punctuation.separator.emboss",
"match": "[:|,]"
},
{
"name": "punctuation.section.brackets.emboss",
"match": "\\\\[|\\\\]"
},
{
"name": "punctuation.section.parens.emboss",
"match": "\\\\(|\\\\)"
},
{
"name": "punctuation.accessor.emboss",
"match": "\\\\."
}
]
}
806d95f to
2e4649e
Compare
1 participant
Resolves #209
Summary
I have completed the task to provide a syntax highlighting definition for GitHub as described in Issue #209.
Root Cause & Approach
GitHub relies on the Linguist library to detect and highlight source code. Linguist, in turn, consumes TextMate grammars (
.tmLanguageor.tmLanguage.json). Since Emboss is a custom language, providing an official TextMate grammar in the repository is the standard way to support GitHub syntax highlighting (which can then be pulled via a PR to GitHub's Linguist).Files Changed
I created a new directory
integration/textmate/and added the following files:integration/textmate/emboss.tmLanguage.json: The complete TextMate grammar for Emboss, mapping keywords, built-ins, types, constants, numbers, operators, and documentation comments.integration/textmate/README.md: Documentation explaining that this directory holds the syntax grammar for tools that support TextMate grammars, including GitHub Linguist and VS Code.integration/textmate/textmate_test.py: A robust Python unit test that validates the structure and correct parsing of theemboss.tmLanguage.jsondefinition.integration/textmate/BUILD: A Bazel build file to integrate the new unit test into the existing test suite, ensuringbazel test //...will catch invalid JSON edits going forward.Validation & Test Results
venvand manually validated the test suite (python3 -m unittest integration/textmate/textmate_test.py), successfully checking that theemboss.tmLanguage.jsoncan be cleanly parsed as JSON with the correct TextMate root properties (scopeName,name,fileTypes).fix/issue-209with the commit messageFix #209: Syntax Highlighting for GitHub. The working tree is clean.Validation