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

Syntax improvements for defs and symbols#75

Open
tonsky wants to merge 8 commits into
atom:masterfrom
tonsky:master
Open

Syntax improvements for defs and symbols#75
tonsky wants to merge 8 commits into
atom:masterfrom
tonsky:master

Conversation

@tonsky
Copy link
Copy Markdown
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

Comment thread grammars/clojure.cson
'1':
'name': 'keyword.control.clojure'
'end': '(?=\\))'
'name': 'meta.definition.global.clojure'
Copy link
Copy Markdown
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.

Comment thread grammars/clojure.cson Outdated
'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
Copy Markdown
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.

Comment thread spec/clojure-spec.coffee Outdated
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
Copy Markdown
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
Copy Markdown
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

Comment thread grammars/clojure.cson
'include': '#sexp'
}
{
'match': '(?<=\\()(.+?)(?=\\s|\\))'
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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