Refactor scoping to better match the TextMate guidelines

[svanharmelen]

• Improve scope names used
• Improve regexps to better match according to the language specs
• Prevent that everything that is not matched, is matched as a variable
• Update and fix existing tests (some turned out to be broken even before I started with this refactor, causing them to always return success while they actually failed)
• Add more tests

Even through functions are still scoped as keyword.function.go and not as storage.type.function.go, this PR still fixes issue #62 IMO. I've looked again and again at the language specs and also at other existing syntax highlighting packages for Go, and they all (Sublime, Emacs, Vim) seem to threat these (chan, const, func, interface, map, struct, type and var) as keywords instead of support.types that are used for these in most other languages.

So I think and believe that most (if not all) Go developers would feel alienated if we changed that for Atom. And to do that only because it seems to be closer to the TextMate guidelines feels a little weird. Next to it being questionable when looking at the language specs. So in the end I decided to keep these specific ones as keywords which should give Go developers the experience they expect and are used to.

[MaximSokolov]

Due your PR I realised that we need punctuation.other for "standalone" punctuation. punctuation.definition looks ambiguous without information of what it define(e.g. punctuation.definition.parameters for round brackets)

{
  'match': '\\{|\\}'
  'name': 'punctuation.other.bracket.curly.go'
}

Same for comma:

{
  'match': ','
  'name': 'punctuation.other.comma.go'
}

Also most of languages use separator instead of delimiter for comma (e.g punctuation.separator.parameters.comma) I thought delimiteris used for separating parts of one entity (e.g. . in atom.workspace) but I am not sure now. Seeems only Go and JS/Coffescript use delimiter scope. I will check it again a bit later then add it to the doc.

I've looked again and again at the language specs and also at other existing syntax highlighting packages for Go, and they all (Sublime, Emacs, Vim) seem to threat these (chan, const, func, interface, map, struct, type and var) as keywords instead of support.types that are used for these in most other languages.

I suppose Sublime use keyword for them because it based on same TextMate Bundle. AFAIK Emacs and Vim don't use TextMate convention for their grammar. Do these editors use the same color for "storage" and "keyword" only for Go?

And to do that only because it seems to be closer to the TextMate guidelines feels a little weird. Next to it being questionable when looking at the language specs.

Do you mean Go Programming Language Specification? TextMate manual mentions about it:

instead of putting everything below keyword (as your formal language definition may insist)...

However I disagree with second part of statement:

...you should think “would I want these two elements styled differently?” and if so, they should probably be put into different root groups.

I personally like same color for storage and keyword. But if I want to download a syntax theme and I see yellow color for storage on the screenshot I would expect that all function, fn, func, -> are the same color — yellow, right?

Seti Syntax

The second reason why these symbols should be storage.type is that scopes are more than just syntax highlighting as said @mnquintana :

... scopes are used for far more than just syntax highlighting - since they're what give raw text semantic structure in Atom, they're also used for things like autocomplete and static analysis...

So I think these symbols should be storage.type I don't known when (nearest Atom 2?) and how (I don't like an idea of creating a fork) we should change them.

[svanharmelen]

Updated the PR after discussing with @MaximSokolov on Slack...

[svanharmelen]

@MaximSokolov as you seem to be the only one that reviewed this one... Are you now OK with merging this? I updated the punctuation... related stuff we talked about and I think we discussed and agreed on the rest right?

[MaximSokolov]

var should be keyword.variable.go Otherwise styles for keyword.operator can affect on "var" symbol

[svanharmelen]

👍 will update the PR accordingly...

[MaximSokolov]

I want to check that fixing variable matching wouldn't affect on syntax highlighting. I notify when I would be sure about it. The rest seems good to me 👍

[MaximSokolov]

Sub-scope should be var instead of variable because it match "var" symbol. var is not a variable. In addition 'keyword.var.go' will not cause misunderstanding because of variable root group.

[svanharmelen]

Check... I will update the PR so the scope names match the exact keyword (e.g. keyword.func.go, keyword.chan.go, keyword.var.go)

[MaximSokolov]

I am wrong. Since storage.type.function is used for function, ->, etc. keyword.function.go, keyword.channel.go, keyword.variable.go are correct scopes too. Actually it can be keyword.variable.<token>.go i.e. keyword.variable.var.go but it looks excessively.

[svanharmelen]

That was my initial reasoning too... So I'll keep this as is.

[MaximSokolov]

Using One Dark syntax theme for this code(which wouldn't compile):

package main

func main() {
    t = template.Must(t.ParseGlob("templates/*.tmpl"))
    err := t.Execute(os.Stdout, nil)
    if err != nil {
        log.Fatalf("template execution: %s", err)
    }
}

Before:

After:

main is entity.name.package now (which is correct)
t, template, err are not tokenized now (variable.other?)
os, log are not tokenized now (support.class?)
Stdout are not tokenized now (don't known what scope should be)
%s is constant.other.placeholder.go(I guess it's correct scope) Was: constant.character.escape.go

[svanharmelen]

Yep, it does change the highlighting (as I would expect). But I believe it does so for the better...

It makes what we currently can scope more correct (it doesn't just assign everything the variable.other.go scope) and it also shows better what isn’t scoped (e.g. things we can still work on to improve and extent in the language package).

So if nothing else, I would like to proceed and merge this one for now... Of course that doesn't mean it's now a 100% done and dusted, but we made a step in the right direction 😉

[MaximSokolov]

So if nothing else, I would like to proceed and merge this one for now

👍

[svanharmelen]

Check... Thanks for all the (Slack) discussion 😉 and here goes...

[betawaffle]

Now var highlights as a variable. :(

[svanharmelen]

@betawaffle do you have an example?

[MaximSokolov]

In One Dark syntax theme for example.

[MaximSokolov]

keyword.variable.go ->keyword.var.go
keyword.constant.go -> keyword.const.go

[svanharmelen]

@betawaffle thanks for reporting this! Should be fixed in version 0.39.0 (just released)...