Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Fix tokenizing of various keywords within ternary expression #228

Merged
merged 2 commits into from
Sep 25, 2015
Merged

Fix tokenizing of various keywords within ternary expression #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2538ed3
Fix tokenizing of various keywords within ternary expression
mislav Sep 21, 2015
246bfea
Add tests for different positions within ternary expressions
mislav Sep 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 41 additions & 17 deletions grammars/javascript.cson
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,12 +468,16 @@
]
}
{
'match': '(?<!\\.)\\b(yield)(?!\\s*:)\\b(?:\\s*(\\*))?',
'match': '(?<!\\.)\\b(yield)(?!\\s*:)\\b(?:\\s*(\\*))?|(?<=\\?)(?:\\s*)(yield)(?=\\s*:)',
'captures':
'1':
'name': 'keyword.control.js'
'2':
'name': 'storage.modifier.js'
'3':
'name': 'keyword.control.js'
'4':
'name': 'storage.modifier.js'
'name': 'meta.control.yield.js'
}
{
Expand All @@ -489,28 +493,44 @@
'name': 'keyword.operator.js'
}
{
'match': '(?<!\\.)\\btrue(?!\\s*:)\\b'
'name': 'constant.language.boolean.true.js'
}
{
'match': '(?<!\\.)\\bfalse(?!\\s*:)\\b'
'name': 'constant.language.boolean.false.js'
'match': '(?<!\\.)\\b(true|false)(?!\\s*:)\\b|(?<=\\?)(?:\\s*)(true|false)(?=\\s*:)'
'captures':
'1':
'name': 'constant.language.boolean.$1.js'
'2':
'name': 'constant.language.boolean.$2.js'
}
{
'match': '(?<!\\.)\\bnull(?!\\s*:)\\b'
'name': 'constant.language.null.js'
'match': '(?<!\\.)\\b(null)(?!\\s*:)\\b|(?<=\\?)(?:\\s*)(null)(?=\\s*:)'
'captures':
'1':
'name': 'constant.language.null.js'
'2':
'name': 'constant.language.null.js'
}
{
'match': '(?<!\\.)\\b([A-Z][A-Z0-9_]+)(?!\\s*:)\\b'
'name': 'constant.other.js'
'match': '(?<!\\.)\\b([A-Z][A-Z0-9_]+)(?!\\s*:)\\b|(?<=\\?)(?:\\s*)([A-Z][A-Z0-9_]+)(?=\\s*:)'
'captures':
'1':
'name': 'constant.other.js'
'2':
'name': 'constant.other.js'
}
{
'match': '(?<!\\.)\\b(super|this)(?!\\s*:)\\b'
'name': 'variable.language.js'
'match': '(?<!\\.)\\b(super|this)(?!\\s*:)\\b|(?<=\\?)(?:\\s*)(super|this)(?=\\s*:)'
'captures':
'1':
'name': 'variable.language.js'
'2':
'name': 'variable.language.js'
}
{
'match': '(?<!\\.)\\b(debugger)(?!\\s*:)\\b'
'name': 'keyword.other.js'
'match': '(?<!\\.)\\b(debugger)(?!\\s*:)\\b|(?<=\\?)(?:\\s*)(debugger)(?=\\s*:)'
'captures':
'1':
'name': 'keyword.other.js'
'2':
'name': 'keyword.other.js'
}
{
'match': '(?<!\\$)\\b(Anchor|Applet|Area|Array|Boolean|Button|Checkbox|Date|document|event|FileUpload|Form|Frame|Function|Hidden|History|Image|JavaArray|JavaClass|JavaObject|JavaPackage|java|Layer|Link|Location|Math|MimeType|Number|navigator|netscape|Object|Option|Packages|Password|Plugin|Radio|RegExp|Reset|Select|String|Style|Submit|screen|sun|Text|Textarea|window|XMLHttpRequest)\\b'
Expand Down Expand Up @@ -545,8 +565,12 @@
'name': 'support.function.js'
}
{
'match': '(?<!\\.)\\b(module|exports|__filename|__dirname|global|process)(?!\\s*:)\\b'
'name': 'support.variable.js'
'match': '(?<!\\.)\\b(module|exports|__filename|__dirname|global|process)(?!\\s*:)\\b|(?<=\\?)(?:\\s*)(module|exports|__filename|__dirname|global|process)(?=\\s*:)'
'captures':
'1':
'name': 'support.variable.js'
'2':
'name': 'support.variable.js'
}
{
'match': '\\b(Infinity|NaN|undefined)\\b'
Expand Down
53 changes: 53 additions & 0 deletions spec/javascript-spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,33 @@ describe "Javascript grammar", ->
{tokens} = grammar.tokenizeLine('with')
expect(tokens[0]).toEqual value: 'with', scopes: ['source.js', 'keyword.control.js']

map =
Copy link
Contributor

@winstliu winstliu Sep 23, 2015

Choose a reason for hiding this comment

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

Can you make it so the test itself isn't looped, only the contents within the test? So

it "...", ->
  map =
    ...
  for keyword, scope of map
    ...

If necessary you can keep map out of the tests to avoid duplication.

Copy link
Contributor Author

@mislav mislav Sep 24, 2015

Choose a reason for hiding this comment

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

I like how the loop generates multiple tests because when tokenizing of one keyword breaks, only its test will fail and not the entire block. This was useful for me in development when I wrote these tests first and changed one by one regular expression to get them passing.

Copy link
Contributor

@winstliu winstliu Sep 25, 2015

Choose a reason for hiding this comment

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

only its test will fail and not the entire block

I think you've actually convinced me to start writing new tests this way 😄!

super: 'variable.language.js'
this: 'variable.language.js'
null: 'constant.language.null.js'
true: 'constant.language.boolean.true.js'
false: 'constant.language.boolean.false.js'
debugger: 'keyword.other.js'
exports: 'support.variable.js'
__filename: 'support.variable.js'

for keyword, scope of map
do (keyword, scope) ->
it "does not tokenize `#{keyword}` when it is an object key", ->
{tokens} = grammar.tokenizeLine("#{keyword}: 1")
expect(tokens[0]).toEqual value: keyword, scopes: ['source.js']
expect(tokens[1]).toEqual value: ':', scopes: ['source.js', 'keyword.operator.js']

it "tokenizes `#{keyword}` in the middle of ternary expressions", ->
{tokens} = grammar.tokenizeLine("a ? #{keyword} : b")
expect(tokens[2]).toEqual value: ' ', scopes: ['source.js']
expect(tokens[3]).toEqual value: keyword, scopes: ['source.js', scope]

Copy link
Contributor

@winstliu winstliu Sep 23, 2015

Choose a reason for hiding this comment

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

Just to be comprehensive can you also add a test for "a ? b : #{keyword}"?

Copy link
Contributor Author

@mislav mislav Sep 24, 2015

Choose a reason for hiding this comment

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

Sure, good call

it "tokenizes `#{keyword}` at the end of ternary expressions", ->
{tokens} = grammar.tokenizeLine("a ? b : #{keyword}")
expect(tokens[4]).toEqual value: ' ', scopes: ['source.js']
expect(tokens[5]).toEqual value: keyword, scopes: ['source.js', scope]

describe "built-in globals", ->
it "tokenizes them as support classes", ->
{tokens} = grammar.tokenizeLine('window')
Expand Down Expand Up @@ -336,6 +363,19 @@ describe "Javascript grammar", ->
expect(tokens[4]).toEqual value: 'systemLanguage', scopes: ['source.js', 'support.constant.js']
expect(tokens[5]).toEqual value: ';', scopes: ['source.js', 'punctuation.terminator.statement.js']

it "does not tokenize constants when they are object keys", ->
{tokens} = grammar.tokenizeLine('FOO: 1')
expect(tokens[0]).toEqual value: 'FOO', scopes: ['source.js']
expect(tokens[1]).toEqual value: ':', scopes: ['source.js', 'keyword.operator.js']

it "tokenizes constants in the middle of ternary expressions", ->
{tokens} = grammar.tokenizeLine('a ? FOO : b')
expect(tokens[3]).toEqual value: 'FOO', scopes: ['source.js', 'constant.other.js']

Copy link
Contributor

@winstliu winstliu Sep 23, 2015

Choose a reason for hiding this comment

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

Same thing here, a test for 'a ? b : FOO' would be nice.

it "tokenizes constants at the end of ternary expressions", ->
{tokens} = grammar.tokenizeLine('a ? b : FOO')
expect(tokens[5]).toEqual value: 'FOO', scopes: ['source.js', 'constant.other.js']

describe "ES6 string templates", ->
it "tokenizes them as strings", ->
{tokens} = grammar.tokenizeLine('`hey ${name}`')
Expand Down Expand Up @@ -399,6 +439,19 @@ describe "Javascript grammar", ->
expect(tokens[0]).toEqual value: 'yield', scopes: ['source.js', 'meta.control.yield.js', 'keyword.control.js']
expect(tokens[2]).toEqual value: '*', scopes: ['source.js', 'meta.control.yield.js', 'storage.modifier.js']

it "does not tokenize yield when it is an object key", ->
{tokens} = grammar.tokenizeLine('yield: 1')
expect(tokens[0]).toEqual value: 'yield', scopes: ['source.js']
expect(tokens[1]).toEqual value: ':', scopes: ['source.js', 'keyword.operator.js']

it "tokenizes yield in the middle of ternary expressions", ->
{tokens} = grammar.tokenizeLine('a ? yield : b')
expect(tokens[3]).toEqual value: 'yield', scopes: ['source.js', 'meta.control.yield.js', 'keyword.control.js']

it "tokenizes yield at the end of ternary expressions", ->
{tokens} = grammar.tokenizeLine('a ? b : yield')
expect(tokens[5]).toEqual value: 'yield', scopes: ['source.js', 'meta.control.yield.js', 'keyword.control.js']

describe "default: in a switch statement", ->
it "tokenizes it as a keyword", ->
{tokens} = grammar.tokenizeLine('default: ')
Expand Down