Skip to content

Conversation

@subhaze
Copy link
Contributor

@subhaze subhaze commented Feb 12, 2016

Renames Quasi literal to Template literal

  • Renamed literal-quasi definition to literal-string-template
  • Renamed all occurences of quasi in scopes to template
  • Added test cases
  • Scope name changes

@subhaze
Copy link
Contributor Author

subhaze commented Feb 12, 2016

Forgot to add [JavaScript] at the beginning of commit, I can fix commit message if needed.

@wbond
Copy link
Member

wbond commented Feb 12, 2016

Just so I don't get this wrong, could you provide an example of the syntax this highlights? I presume it is ES6. I'd like to add it to the tests so that we are building up a "contract" over time.

@subhaze
Copy link
Contributor Author

subhaze commented Feb 12, 2016

... Sorry I meant to look at the tests and add if something was missing/broke from the change. I'll have to look later, but no prob, will do!

@subhaze
Copy link
Contributor Author

subhaze commented Feb 12, 2016

@subhaze subhaze force-pushed the literal_quasi_to_string_template branch from 6b377ae to 0c50c53 Compare February 12, 2016 22:47
@subhaze
Copy link
Contributor Author

subhaze commented Feb 12, 2016

@wbond updated PR to include additional test case.

@subhaze
Copy link
Contributor Author

subhaze commented Feb 12, 2016

Looking at this it may be better to change entity.template.tag.name to variable.function.template.tag.name.js since it is a function name. This way it's not just the same color as the string, but still keeps its meaning. Thoughts?

@subhaze
Copy link
Contributor Author

subhaze commented Feb 12, 2016

Now that I think about it, it would probably be good to make the template literal multiline in the test. I'm about to be AFK for a bit but will try to add that into the test case soon.

@FichteFoll
Copy link
Collaborator

Ah, I made sure to check the open issues for this before opening my own, but I didn't catch this one.

FWIW, I reported this issue with some more detail and scope name suggestions in #169:

Currently, string interpolation in the form let str = I'm a ${"string"}.; has the following issues:

  1. the meta name is string.quasi. I think this should be string.interpolated.backtick
  2. backtick punctuation gets the scope names punctuation.definition.quasi.begin. Imo it should be punctuation.definition.string.begin.interpolated (and end respectively)
  3. the pattern denotation is marked as punctuation.quasi.element.begin. I suggest punctuation.definition.begin.section.interpolated or similar.
  4. the entire pattern is scoped as entity.quasi.element. Entitiy is definitely wrong here. I'm unsure atm what should be used instead (maybe keyword), but something with embedded would be a good start.

@subhaze, please consider these. If "template" is the official terminology, I can take that as well. I definitely recommend using punctuation.definition.string.* however.

@subhaze
Copy link
Contributor Author

subhaze commented Feb 13, 2016

@FichteFoll I agree on the 'element' bits completely. Since we're embedding expressions I've adjusted to scope names for this are to reflect that. Thought I'm not sure about template.expression.embedded.js

I'm pretty new to scope naming standards so more feedback is very welcomed.

Also, I was going to update the tests to check for multiline (it currently works) but it seems that the syntax tests wont allow for this as far as I can tell. In these template literals you can't break up multilines with comments... so... not sure how to test for multilines other than just adding two separate tests.

@subhaze subhaze force-pushed the literal_quasi_to_string_template branch from b624e1e to af5a8a1 Compare February 13, 2016 18:34
@subhaze
Copy link
Contributor Author

subhaze commented Feb 13, 2016

@wbond in hindsight I felt I shouldn't have moved this down to literal-string, just so it's easier to see what was actually changed and to help reduce any kind of merge conflicts that may happen later if this PR is brought it.

This PR should now provide a better story in the visual diff of what has actually changed.

@FichteFoll
Copy link
Collaborator

I would:

  • entity.name.function.template.tag.js → variable.function.tagged-template.js,
    because it is a function call, not a function definition
  • punctuation.definition.template.begin.js → punctuation.definition.string.template.begin.js (and end),
    because we are inside a string scope and the selector string punctuation.definition.string (which I use for all strings) would match then.
  • punctuation.template.expression.begin.js → punctuation.definition.template-expression.begin.js (and end),
    because currently only punctuation.definition|separator|section|terminator are commonly known
    FWIW, Ruby uses punctuation.section.embedded.ruby
  • meta_scope: template.expression.embedded.jsmeta_scope: meta.template.expression.js
    and add meta_content_scope: source.js.embedded.expression or embedded.js.expression (inspired by PHP syntax),
    because only the content is javascript and these two variants have been used in other syntaxes before (except for the "expression" thing, I just made that up)
    Ruby uses source.ruby.embedded.source

Maybe compare against other languages that have template/interpolated strings.

@subhaze
Copy link
Contributor Author

subhaze commented Feb 15, 2016

Thanks, I'll review the above (hopefully later today) and adjust the PR.

  - Renamed literal-quasi definition to literal-string-template
  - Renamed all occurences of quasi in scopes to template
  - Added test cases
  - Updated scope names
@subhaze subhaze force-pushed the literal_quasi_to_string_template branch from af5a8a1 to abdb3db Compare February 15, 2016 21:00
@subhaze
Copy link
Contributor Author

subhaze commented Feb 15, 2016

Updated scopes from notes above, rebased this branch on top of the current Packages master branch.

@subhaze subhaze changed the title Renames Quasi literal to Template literal [JavaScript] Renames Quasi literal to Template literal Feb 15, 2016
@wbond
Copy link
Member

wbond commented Feb 16, 2016

Thank you both for the time spent working out improvements for this!

wbond added a commit that referenced this pull request Feb 16, 2016
[JavaScript] Renames Quasi literal to Template literal
@wbond wbond merged commit 8508cf1 into sublimehq:master Feb 16, 2016
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.

4 participants