billymoon / Stylus

Stylus Package for Sublime Text 2 / 3
Other
129 stars 31 forks source link

Big .tmLanguage regressions... #21

Closed nfour closed 10 years ago

nfour commented 10 years ago

My last commit 4 months ago #19 and another before it #10 added a lot of additions to the tmLangauge which @grassator has since reverted.

Eg.

    text-shadow:    0 -1px 0 #637083,
                0 1px 0 #637083,
                1px 0 0 #637083,
                -1px 0 0 #637083

(any multiline stuff)

Is completely broken and un-scoped.

    -webkit-transition  : arguments
    -moz-transition : arguments

In the two above lines, the first has the : highlighted as keyword.operator.stylus, the other has no scope at all. Apparently tabs are different to spaces, nice.

:, , {} and other delimiters are not highlighted anymore or simply use punctuation.start which just isn't highlighted in most color schemes.

Somehow #aaa4 and #aaaaaa44 compiles to a variable scoping. I dont even. It's supposed to a RGBA hex value. Additionally, R G B and A value reverted as well.

    .table
        width: 100%

    input[type="text"], input[type="password"], textarea
        width: 100%

In the above, input etc. is just unscoped, reverting significant attribute scoping. Probably a lot more broken that I haven't noticed.

Exactly what was the point of this? I'm pretty tired of fixing this plugin's .tmLanguage when shit like this goes down out of the blue. I'm assuming it's down to the addition of the .yml, because editing XML is so difficult.

I noticed some things have been retained. I suggest we revert to old .tmLangauge or migrate previous changes to new tmLanguage. See #10 and #19 for what has been regressed. #20 is of course the tip of the iceberg.

grassator commented 10 years ago

I'm sorry that this update affected you in a negative way and I will try to fix mentioned regressions as soon as possible. And I will add these samples to a test file to reduce possibility of future regressions.

Thanks for reporting.

As to reason why syntax definition changed, it's important to understand that everyone's usage patterns are different. In previous version, for example, there was virtually no highlighting for functions or dealing with variables, especially at the top level. Unfortunately due to very relaxed syntax of stylus it was impossible to add such functionality to the old syntax definition.

To be able to do this I had to rewrite whole thing using more PEG-like format. It has a certain drawback – with such approach it is required that everything is to be defined explicitly within certain scopes and you can't just throw in some general regular expressions that are matched on whole file. But on the plus side right now the definition is much more readable, extensible and supports highlighting for what was previously impossible to highlight at all.

nfour commented 10 years ago

I appreciate that you re-wrote everything for a good reason. I'll contrib some improvements if they present themselves. Carry on :).

grassator commented 10 years ago

All right, If I haven't missed anything, then all the improvements from your previous commits should be already in master. If you find anything new or what I've missed from your list please just let me know.

One thing I deliberately did not implement is highlighting of individual braces and other punctuation symbols. They were not meant to have meta.* scope (based on official naming conventions). But the final word on this should be from @billymoon, as he's the owner of this package after all.

If there won't be any additional changes I will close this ticket in a few days.

nfour commented 10 years ago

I believe there should be some scope for all the delimiters; it seems ,, : and {} are the only delimiters/punctuation unscoped, as it stands. The more scopes, even if obscure, provide maximum functionality as not to limit a colorscheme's depth.

I've found this odd throughout sublime's default schemes and tmLanguages. All big IDE's always highlight common elements like punctuation or allow one to, in all langs, yet you look at the default CSS for ST3 and it's like; #id { }, the first brace is given a scope, but the last brace is completely scopeless. PHP's was another horrible example of syntax highlight laziness. I suppose it's a remanent from textmate, a limitation or laziness and apathy by users.

billymoon commented 10 years ago

@grassator I am happy for you to go ahead and use your best judgement in all matters relating to maintaining this package. I will get notifications of anything going on, and if I feel I have something to contribute, I will chime in.

@nfour sorry I was not more diligent in checking your changes were not brought forward into our updated code.

nfour commented 10 years ago

The property value in

font-family: Georgia
box-sizing: border-box

(Georgia, border-box) is being given variable.scope.stylus when it should have a property-value scope.

I noticed in the .yml

    variable:
        match: '([\w$-]+)'
        name: variable.other.stylus

That seems awfully blanketing, doesn't it? Any unmatched words are becoming variable scope. I guess the whole point of the re-write was to support this, though, really, it's a maintenance nightmare accounting for every property value. Not saying it's a silly idea though, as it could actually be ideal as a sort of syntax check on acceptable property values for each rule (if it's not being highlighted as a var, then it's correct!). It does mean every single experimental CSS prop needs its accompanying value scoping or else if anyone dares to venture into something not in this repo, things look broken, so I'm not sure the whole white-listy way is worth it. If you're going to highlight something as a variable, maybe you should be 100% sure it's a variable first.

grassator commented 10 years ago

@nfour I completely agree that everything except may be for white space should have some scope so in 3600c32 I've added punctuation.* scope to every appropriate symbol I could find. If something is still missing please let me know.

grassator commented 10 years ago

@nfour I don't agree that maintenance of property values is so tedious. Yes, CSS standard changes, but it doesn't change that often, and if a feature is experimental, and you are using it, well then you should expect lack of support from the tools you use. And this is a proper way of approaching things that is used in real IDEs.

Now back to examples you've mentioned.

box-sizing constants indeed were missing from language syntax, so thanks for reporting, I've added them and some more in latest commit. If you notice anything else missing, let me know and I will add it. May be I will even dump official CSS grammar or MDN to make sure everything is there.

Case of font-family is the only tricky one as far as I can tell, but highlighting everything unrecognized that resembles a variable as a variable scope is correct in case of Stylus. The way Stylus works is that it considers every literal a variable, and then if at runtime that variable resolves to undefined then it is transformed into a literal with it's name. So this piece of Stylus code:

Times = Arial
a {
  font-family: Times;
}

will yield this CSS:

a {
  font-family: Arial;
}

And in this case Arial can also be a variable defined in another file or through Node.js, and resolved to something else at runtime. Current highlighting warns about such behavior and if you want to avoid it, then using strings to define font families is a good idea.

nfour commented 10 years ago

Good point with how Stylus handles variables, and :+1: for punctuation.*

If I see any more syntax fixes I will just update highlight-test.stylus with an example and it can be dealt with at leisure.