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

Syntax improvements for defs and symbols #75

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Syntax improvements for defs and symbols #75

wants to merge 8 commits into from

Conversation

tonsky
Copy link
Contributor

@tonsky tonsky commented Nov 27, 2017

Description of the Change

In top-level definitions like

(defn f [] (+ 1 2))

we shouldn’t mark the whole expression as "meta.definition.global.clojure". Instead, we should mark only f (function name) as "entity.name.global.clojure meta.definition.global.clojure". This is similar to how other syntaxes do it, e.g. JavaScript and Python.

In addition to that, this PR contains a couple of fixes to existing regexps:

  • Recognize single-char symbols
  • Symbols can’t start with [0-9#']
  • Symbols can contain [#$%&'|]
  • When defining dynamic var (def *var* ...), capture *var* as "entity.name.global" as well

Alternate Designs

Not applicable

Benefits

  • Better syntax highlighting
  • Syntax highlighting working closer to other languages

Possible Drawbacks

Existing themes that were designed specifically with Clojure syntax in mind might break (I doubt ones exist, though)

Applicable Issues

Not applicable


This change is Reviewable

'beginCaptures':
'1':
'name': 'keyword.control.clojure'
'end': '(?=\\))'
'name': 'meta.definition.global.clojure'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this meta should be kept and the actual function changed to just entity.name.global.clojure.

'match': '([\\w\\.\\-\\_\\:\\+\\=\\>\\<\\!\\?\\*][\\w\\.\\-\\_\\:\\+\\=\\>\\<\\!\\?\\*\\d]+)'
'name': 'entity.global.clojure'
'match': '([A-Z_a-z!$%&+\\-.:<=>?|*][0-9#\'A-Z_a-z!$%&+\\-.:<=>?|*]*)'
'name': 'meta.definition.global.clojure entity.name.global.clojure'
Copy link
Contributor

Choose a reason for hiding this comment

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

Scopes != CSS classes and while this may look correct in the DOM it will appear as one scope internally.

for symbol in symbols
{tokens} = grammar.tokenizeLine "(#{macro} #{symbol} 'bar)"
expect(tokens[1]).toEqual value: macro, scopes: ["source.clojure", "meta.expression.clojure", "keyword.control.clojure"]
expect(tokens[3]).toEqual value: symbol, scopes: ["source.clojure", "meta.expression.clojure", "meta.definition.global.clojure entity.name.global.clojure"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Like so.

@tonsky
Copy link
Contributor Author

tonsky commented Dec 6, 2017

Agree. Looks like in other languages meta.definition tends to capture the whole expression, while entity.name is usually just class/function name. I made the appropriate changes. I also think entity.name.definition.clojure is a better name than entity.name.global.clojure because meta is called meta.defintion.global.clojure, with definition being the main semantic part and global just a modifier

@@ -322,17 +322,6 @@
{
'include': '#sexp'
}
{
'match': '(?<=\\()(.+?)(?=\\s|\\))'
Copy link
Contributor

Choose a reason for hiding this comment

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

And why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was placing the same entity.name.function to all function calls inside defn. Because the token is the same, there was no way to distinguish between function declaration and function call. I checked with other syntaxes and they seem to put entity.name.* on definitions only as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think this is worth keeping. I checked language-javascript, language-java, and language-php and all use entity.name.function for function declarations and function calls but have an additional meta scope to differentiate between whether it's a declaration or call (usually meta.function vs meta.{function|method}-call).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe you’re right. I was looking at Ruby. Returned it back

@winstliu
Copy link
Contributor

winstliu commented Dec 7, 2017

I don't think that patterns: include $self block should actually be there. Since before it was at the wrong indentation and not doing anything and similar grammars don't have it.

@tonsky
Copy link
Contributor Author

tonsky commented Dec 7, 2017

Actually, I think I messed up the indentation when I copied it back. 'patterns' was on the same level as 'captures' and I accidentally made it deeper. Would it make more sense if I return it the way it was before?

@winstliu
Copy link
Contributor

winstliu commented Dec 7, 2017

@tonsky the previous indentation, with the patterns at the same level as captures, also doesn't make sense. That syntax is only for begin/end patterns and not regular matches.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants