Skip to content
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

add % as a dedicated punctuation scope #72

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

dustypomerleau
Copy link
Contributor

The way this Elixir syntax is written doesn't specifically address capture groups for maps or structs. As a result, somehow % was left out of all scopes, including punctuation. I considered writing a proper struct/map regex to capture all the scopes (and I am still happy to do that), but this was the simplest solution to allow % to match other punctuation (or be themed in general). Since Elixir doesn't use % as a modulo operator, it seems safe to simply give it the one scope in all contexts. Giving it a separate scope allows theme designers to contrast it with the neighboring curly braces if they prefer.

Struct names are currently scoped as variable.other.constant.elixir. That scope was intended for references to module names, but it ends up capturing any capitalized name, including structs. That doesn't bother me, but if others prefer to theme struct names independently, I can add the full regex with captures.

Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

Thanks for the write-up and the PR!

Struct names are currently scoped as variable.other.constant.elixir. That scope was intended for references to module names, but it ends up capturing any capitalized name, including structs. That doesn't bother me, but if others prefer to theme struct names independently, I can add the full regex with captures.

This seems fine to me as-is since structs are always modules.

@lukaszsamson
Copy link
Collaborator

This seems fine to me as-is since structs are always modules.

That's not entirely true. Structs are compile time atoms or aliases. This is valid elixir:

%:non_elixir_module{}

@dustypomerleau
Copy link
Contributor Author

dustypomerleau commented Jun 3, 2020

I guess the question is: Given the frequency with which an atom might be used in a struct name, are you happy for it to simply be highlighted as an atom (constant.other.symbol.elixir)? I have no idea how often this comes up.

@jayjun
Copy link
Contributor

jayjun commented Jun 3, 2020

I’m not bothered by this because modules in remote function calls are scoped the same now.

Foo.bar()  # variable.other.constant.elixir
:foo.bar() # constant.other.symbol.elixir

@dustypomerleau
Copy link
Contributor Author

dustypomerleau commented Jun 4, 2020

Assuming you don't need a meta scope wrapping the entire struct, it's easy enough to do something like:

(%) punctuation.definition.map.elixir
(:)? punctuation.definition.constant.elixir
([A-z](\w|\.)*)? variable.other.constant.elixir
\{ uncaptured

It's just a question of whether it's unnecessary complexity, given it occurs rarely.

@axelson
Copy link
Member

axelson commented Jun 4, 2020

Given the frequency with which an atom might be used in a struct name, are you happy for it to simply be highlighted as an atom (constant.other.symbol.elixir)?

I think so. Since structs are an elixir-concept, there is almost no reason to define a struct as a lowercase atom. I'm going to go ahead and merge this.

@axelson axelson merged commit 054d828 into elixir-lsp:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants