The Wayback Machine - https://web.archive.org/web/20201212233845/https://github.com/atom/language-clojure/pull/39
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

Fix tokenization of sexp and map nested at beginning of another sexp #39

Merged
merged 3 commits into from Mar 15, 2016

Conversation

@bytebutt
Copy link
Contributor

@bytebutt bytebutt commented Mar 4, 2016

I'm new to Clojure, but something that jumped out at me while learning in Atom was parentheses being colored as function characters if they immediately followed another parenthesis. As someone else mentioned in #35, this also happens when maps are used as functions.

I've made a small tweak to the entity.name.function.clojure match to exclude ( and { as function characters. I've included screenshots below of the syntax highlighting before and after my change.

Before
before

After
after

@bytebutt bytebutt changed the title Exclude ( and { as function start characters Exclude ( and { as function characters Mar 4, 2016
@bytebutt
Copy link
Contributor Author

@bytebutt bytebutt commented Mar 4, 2016

Updated title and description to make it more clear that my change excludes ( and { from the function match entirely, not just at the very start.

@@ -274,7 +274,7 @@
'include': '#vector'
}
{
'match': '(?<=\\()(.+?)(?=\\s|\\))'
'match': '(?<=\\()([^\\(\\{]+?)(?=\\s|\\))'

This comment has been minimized.

@50Wliu

50Wliu Mar 4, 2016
Member

\\{ can simply be {.

This comment has been minimized.

@pjago

pjago Nov 23, 2016

How about a match for sets?
As in ;=> (#{:a :b} k)

@50Wliu
Copy link
Member

@50Wliu 50Wliu commented Mar 4, 2016

This will need some specs.

@bytebutt
Copy link
Contributor Author

@bytebutt bytebutt commented Mar 7, 2016

Specs aside, I think the same result can be achieved by adding the following above the entity.name.function.clojure match:

{
  'include': '#map'
}
{
  'include': '#sexp'
}

Would you consider this preferable to the current solution I've proposed above? I think it's more readable, but I'm not sure if using include introduces an additional performance impact or has some other non-obvious negative consequence (especially with respect to the recursive reference to sexp).

@50Wliu
Copy link
Member

@50Wliu 50Wliu commented Mar 7, 2016

I think your new method would definitely be preferred. I'm not aware of there being any performance impacts with recursive rules - some other languages use them as well and are fine.

@bytebutt bytebutt changed the title Exclude ( and { as function characters Fix tokenization of sexp and map nested at beginning of another sexp Mar 15, 2016
@bytebutt
Copy link
Contributor Author

@bytebutt bytebutt commented Mar 15, 2016

I've redone my grammar change to use included patterns instead of modifying the function match. I've also added some specs for the improved tokenization.

50Wliu added a commit that referenced this pull request Mar 15, 2016
Fix tokenization of sexp and map nested at beginning of another sexp
@50Wliu 50Wliu merged commit ae01eb2 into atom:master Mar 15, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50Wliu
Copy link
Member

@50Wliu 50Wliu commented Mar 15, 2016

Looks good, thanks!

Slowki pushed a commit to Slowki/hy.tmLanguage that referenced this pull request Sep 14, 2018
Fix tokenization of sexp and map nested at beginning of another sexp
Slowki pushed a commit to Slowki/hy.tmLanguage that referenced this pull request Sep 14, 2018
Fix tokenization of sexp and map nested at beginning of another sexp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.