-
Notifications
You must be signed in to change notification settings - Fork 232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Work in Progress] Initial work on native sublime syntax support #109
Conversation
Thanks for working on this @skyronic ! I tried it out locally, but running into a few issues:
|
This syntax handles attributes like the core HTML syntax, but the core HTML syntax has some problems in this regard. I submitted a PR today addressing some of these problems; you might want to take a look at it. The syntax doesn't handle attributes in The way that Vue directives are injected into the HTML syntax seems problematic: <template>
:foo="
</template> Surely that's not supposed to happen. I can see how you're trying to avoid getting into the weeds of the HTML syntax with a simple include, but I don't think it's going to work. I wish I knew of another easy answer that didn't involve copy-pasting the HTML syntax and starting from there, but all I have to recommend is my YAML Macros package. |
@yyx990803 I've tested this out with the latest dev build of sublime. Will test it out with stable version and update the PR. @Thom1729 There are a small set of attributes for One downside is that attributes aren't colored properly so I'm catching the most used attributes and attaching scopes to them as a bit of a hack. Will keep updating this. Will look into the option of embedding html syntax as well. |
@yyx990803 looks like the issue is I was using some features of While this is arguably a better and cleaner way to do it than the older approach, it looks like it's simply not going to be feasible until everyone updates to the latest version. My current idea is to try an approach which works for older and newer versions, and we can consider upgrading to the correct approach later. |
@yyx990803 I've update it to use the backward-compatible approach which should work with the current stable release. Still many language support yet to be done, and there might be bugs. Please do try it out when you've got some time. It should be ready for release soon. There's a new feature But if we move to the |
Thanks for the update @skyronic - I didn't realize Coincidentally, the current syntax still works fine in the stable release and only breaks in the dev build. Maybe we should just go with the new mechanism in this PR as we work on it, and release it as a separate package so that users of both stable and dev can get a working syntax? |
You can modify the Package Control channel record to declare that certain releases are compatible with 3153 and below and others are compatible with 3154 and above. This way, Package Control won't upgrade users on an older Sublime build to a version that depends on In either case, you should make sure you're developing on the dev build, as the internal JavaScript implementation has changed substantially; #107 does not occur at all on older builds. |
(deleted an older comment since @Thom1729's message clarifies my question) Thom: a new question. If a user on an old version of sublime, who doesn't have the extension installed tries to install it, does it fetch the appropriate version? |
Yes; that is my understanding. So if today you released a new version for 3154+, and a user on the stable channel installed this package, they would get the old version, whereas a user on the dev channel would get the new version. |
Hello there. I know this is still in progress, but I thought I would share a couple of the bugs that I've come across while using it. This is for the most recent commit as of posting this. In the screen shot there are 3 bugs:
I've noticed that the third can be fixed by adding a semi-colon after the closing mustache of the export, but not including a semi-colon is also correct (especially since I'm using the standard coding style). |
Semi-relevant question.
What coding style is this? Is there a standard coding style that recommends omitting semicolons? |
A lot of them really, and most teams I've worked with over the last few years omit semicolons. https://standardjs.com/ is one of them. The problem is |
@yyx990803 so here's my investigation on how we can ship a new version. In the package control source code there's some instructions // If your package is only compatible with specific builds of
// Sublime Text, this will cause the package to be hidden from
// users with incompatible versions.
//
// The "tags" key can be true for all valid semantic version tags, or
// can be a prefix string. Only tags in the form
// {prefix}{semantic_version} will be selected. In the example below,
// the entry with "sublime_text": "<3000" will match tags like:
//
// "st2-1.0.0"
// "st2-1.1.0"
//
// The release with "sublime_text": ">=3000" will match tags like:
//
// "st3-1.0.0"
// "st3-1.1.0"
{
"details": "https://github.com/wbond/sublime_alignment",
"releases": [
{
"sublime_text": "<3000",
"tags": "st2-"
},
{
"sublime_text": ">=3000",
"tags": "st3-"
}
]
}, I looked through the package control repository and found a suitable candidate which followed this pattern: {
"details": "https://github.com/SublimeText/NSIS",
"labels": ["language syntax", "nsis", "build system"],
"releases": [
{
"sublime_text": "<3103",
"tags": "st2-"
},
{
"sublime_text": ">=3103",
"tags": "st3-"
}
]
}, I located an old build of Sublime text (3102) and installed package control. I then tried to install that extension and it found the correct version and installed the old format. So here's what I propose:
Does this sound ok? |
@skyronic sounds like a plan! |
Unfortunately, I have no special insight as to when a new stable build will be released. It could be today or it could be several weeks. I would guess that we are likely to have several more dev builds first, as there are some corner use cases for
The |
This reverts commit 3beb4ba.
@z3nz thanks for the failing cases, they are fixed now! |
This looks much improved. Some comments I have at this stage:
|
Thanks for your notes. I've incorporated many changes with the suggestions. Just wanted to confirm a few things:
Yeah. I noticed this is an issue. And would appreciate your insight in helping me figure out a solution. Right now I'm doing it like this:
However, if I do it something like this to only match a html tag
I will end up having to do attribute parsing and re-implementing the whole of As for the |
- changed underscores in names to hyphens - added support for single quote in attribute strings
Wrote a post in the sublime forum asking for inputs regarding this issue. |
It seems like what we need to do, in a nutshell, is extend the HTML syntax with additional functionality. The base syntax highlighting system doesn't make it easy to do this; generally, you'd have to copy the entire HTML syntax and then manually modify it. The best (only) alternative I am aware of is my YAML Macros package. I'll post a tailored example later. |
@Thom1729 my concern is having to manually update and track changes to the "official" sublime text html package. Will wait for your inputs. @yyx990803 these are the issues right now: Some vue related attributes get highlighted outside of a HTML attribute: Attributes in style/script tags etc don't get highlighted (apart from lang) Newlines break the syntax (not sure why this is happening but it should be an easy fix): |
Here's an example: %YAML 1.2
%TAG ! tag:yaml-macros:YAMLMacros.lib.extend:
---
!extend
_base: HTML.sublime-syntax
name: Vue
scope: text.html.vue
hidden: false
contexts: !merge
main: !prepend
- include: vue-tags
tag-attributes: !prepend
- include: vue-directive
vue-tags:
- match: '(?i)(<)(template)\b'
captures:
0: meta.tag.template.begin.html
1: punctuation.definition.tag.begin.html
2: entity.name.tag.template.html
push:
- match: (?i)(</)(template)(>)
captures:
0: meta.tag.template.end.html
1: punctuation.definition.tag.begin.html
2: entity.name.tag.template.html
3: punctuation.definition.tag.end.html
pop: true
- match: '>'
scope: meta.tag.template.begin.html punctuation.definition.tag.end.html
push: mustache-template
- match: ''
push:
- meta_scope: meta.tag.template.begin.html
- match: '(?=>)'
pop: true
- include: tag-attributes
mustache-template:
- match: (?=</template)
pop: true
- match: '{{'
scope: punctuation.definition.template.begin.html
embed: scope:source.js
escape: '}}'
escape_captures:
0: punctuation.definition.template.end.html
- include: main
vue-directive:
- match: \b(v-[\w-]+)\b
scope: entity.other.attribute-name.html
push: js-attribute-value
- match: \B(:|@)([\w-]+)\b
captures:
1: punctuation.definition.attribute.html
2: entity.other.attribute-name.html
push: js-attribute-value
js-attribute-value:
- match: '='
scope: punctuation.separator.key-value.html
set: js-string
- include: else-pop
js-string:
- match: '"'
scope: punctuation.definition.string.begin.html
embed: scope:source.js
escape: '"'
escape_captures:
0: punctuation.definition.string.end.html
- match: "'"
scope: punctuation.definition.string.begin.html
embed: scope:source.js
escape: "'"
escape_captures:
0: punctuation.definition.string.end.html
- include: else-pop This doesn't implement everything, but it does solve the core problem of coherently extending the HTML syntax with new behavior. This example is based on this PR, which fixes some of the HTML syntax's whitespace issues. In order to build, you need a local copy of that syntax. When the core HTML syntax is updated, you would pull a copy of that upstream syntax and rebuild the Vue syntax. This is admittedly inconvenient, but it's certainly less inconvenient than copying the HTML syntax, extending it in-place to handle Vue, and integrating future improvements in the upstream HTML syntax manually in the patched syntax. I am exploring ways to improve the experience of using YAML Macros to extend built-in syntaxes and would welcome input along these lines. I recommend YAML Macros fairly frequently because I frequently run into situations where the only way to extend an existing syntax in a certain way is by copying it and patching the copy. YAML Macros isn't an alternative to this approach, but rather a framework that avoids some of the pain of doing it manually. |
@Thom1729 Unfortunately I was not able to build the example you listed with Yaml Macros package. I get this error:
I've been dogfooding this syntax file for the last 1 week working heavily with vue syntax files and I definitely think it's good enough for a first release. |
@Thom1729 can you make a gist or paste here the yaml that's generated from the macros file? Also is there a way I can run it without sublime's build system? I noticed there's a cli.py file is there a way I can run that and test it out? Would definitely like to do this the right way but considering that sublime can release the dev build anytime now I'd rather have this be released in a usable manner so people on dev builds can try it out and report any other bugs. |
Are you building exactly the example I posted, or an improved version thereof? I can reproduce that error if I remove the |
@Thom1729 Yes I am using the exact one you pasted with Yaml Macros v2.1.0 on OS X With Sublime text 3154 |
You need a copy of this version of the HTML syntax, which includes fixes to some whitespace issues. The PR hasn't been merged into core yet. I was able to reproduce that error by using the current version of the HTML syntax without those fixes. Sorry for the confusion. The YAML Macros |
Hi. Could you also add typescript support please. Just |
@skyronic Is the example working for you now? |
@Thom1729 Sorry was bit busy this week couldn't respond. The core of it works really well, thank you so much for taking the time to put this together. Couple of questions I'm currently working on adding support for other languages. Here's how I'm handling the coffescript: contexts: !merge
main: !prepend
- include: vue-tags
- include: script-langs
tag-attributes: !prepend
- include: vue-directive
script-langs:
- match: (<)(script)\b[^>]*(lang)\s*=\s*([\'\"]coffee[\'\"])\s*.*(>)
captures:
1: punctuation.definition.tag.begin.html
2: entity.name.tag.script.html
3: entity.other.attribute-name.html
4: string.quoted.double.html
5: meta.tag.script.end.html
embed: scope:source.coffee
escape: (</)(script)(>)
escape_captures:
1: punctuation.definition.tag.begin.html
2: entity.name.tag.script.html
3: meta.tag.script.end.html So the downside is the attributes aren't highlighted I can possibly use The other issue is that it's highlighting the HTML outside the template block. This should not be happening normally.
|
Consider this excerpt from the HTML syntax: - match: '(<)((?i:script))\b(?![^>]*/>)(?![^>]*(?i:type.?=.?text/((?!javascript).*)))'
captures:
0: meta.tag.script.begin.html
1: punctuation.definition.tag.begin.html
2: entity.name.tag.script.html
push:
- match: (?i)(-->)?\s*(</)(script)(>)
captures:
0: meta.tag.script.end.html
1: comment.block.html punctuation.definition.comment.html
2: punctuation.definition.tag.begin.html
3: entity.name.tag.script.html
4: punctuation.definition.tag.end.html
pop: true
- match: '(>)\s*(<!--)?'
captures:
1: meta.tag.script.begin.html punctuation.definition.tag.end.html
2: comment.block.html punctuation.definition.comment.html
embed: scope:source.js
embed_scope: source.js.embedded.html
escape: (?i)(?=(-->)?\s*</script)
- match: ''
push:
- meta_scope: meta.tag.script.begin.html
- match: '(?=>)'
pop: true
- include: tag-attributes It only consumes the opening This approach does have some downsides. It can be brittle if there's unexpected whitespace or in other corner cases. The ideal solution would be to have a special set of contexts for handling script tag attributes. You would have one context per language; each context would include
Ah; apparently I've misunderstood the Vue component file structure. Currently, the best way to remedy this would probably be to have two |
Hey @Thom1729 I've made a lot of changes and incorporated your suggestions and yaml macros. Can you take a quick look? Hoping to get this wrapped up in the weekend 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands, the HTML.sublime-syntax
file will be picked up by Sublime, possibly interfering with the core HTML syntax or other third-party HTML syntaxes. This is a problem I ran into myself when I created the YAML Macros examples.
One solution is to add a hidden: true
to HTML.sublime-syntax
. The disadvantage here is that when you replace the file with a newer version from upstream, you have to remember to patch it.
What I'm doing now is simply changing the file extension from .sublime-syntax
to .yaml
. YAML Macros doesn't care one way or the other. The disadvantage here is that PackageDev's sublime-syntax highlighting won't automatically be applied, but this is a minor annoyance.
0: punctuation.definition.string.end.html | ||
- include: else-pop | ||
|
||
langs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of repeated code between these language-specific declarations. Could this be factored out into a macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried making it into a macro, but there were some issues with string escaping. I created a python file similar to the on in your sql examples but I had some issues. Will share that as well.
- include: else-pop | ||
|
||
langs: | ||
- match: (?:^\s+)?(<)((?:script))\b(?=[^>]*lang\s*=\s*(['"])coffee\1?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backreference will trigger Sublime's old, slower regexp engine. This could be rewritten without it for improved performance.
Also, why match (?:^\s+)?
? I don't think that this should be marked with the meta scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed backreference. Not sure why that was even required. Seems to work without the backreference.
Removed the (?:^\s+)?
. I'm not great with regexp and used some snippets from the old syntax where this came from 😅
1: punctuation.definition.tag.begin.html | ||
2: entity.name.tag.script.html | ||
push: | ||
- match: (?i)(-->)?\s*(</)(script)(>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to match the -->
here. Surrounding a script with an HTML comment is an ancient practice that was used to support browsers that hadn't yet implemented script tags. In this day and age, it should never come up, especially when the Vue syntax has to handle non-JavaScript languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. thank you!
1: meta.tag.script.begin.html punctuation.definition.tag.end.html | ||
2: comment.block.html punctuation.definition.comment.html | ||
embed: scope:source.coffee | ||
escape: (?i)(?=(-->)?\s*</script) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment applies to the -->
in the lookahead here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thank you!
- match: '(?=>)' | ||
pop: true | ||
- include: tag-attributes | ||
- match: (?:^\s+)?(<)((?:style))\b(?=[^>]*lang\s*=\s*(['"])sass\1?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about the backreference and the possibly-unnecessary whitespace matching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the whitespace matching. thank you.
pop: true | ||
- include: tag-attributes | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra trailing lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Thank you.
Hey @Thom1729 should be a lot better now please do take a look. Once it's ready we can publish this for the dev channel! :) |
@yyx990803 after a bit of testing it looks like it's working ok. Listing notes on how it could be deployed. Could wait a few more days I reckon, would also appreciate @Thom1729's notes on this if it looks ok. Proposed branching/tagging strategy
Suggested Package json definitionIn
Please be sure to Run the tests I did run them and it seems to be fine though with this code. |
Looks good to me. The files in the examples directory should probably be turned into syntax tests. Also, the giant list of languages practically screams for a macro or two, but YAML Macros 3.0 will hopefully make that much easier. I might pop back in when it's released (hopefully later in the week). |
@skyronic I've given you push access to this repo - feel free to push fixes and release tags as you see fit! |
For #108
What works:
Still to do:
To test this out:
Vue Component.sublime-syntax
file from hereIf you encounter issues with modern JS features, install the Babel Syntax highlight package and disable the built-in JavaScript.