-
Notifications
You must be signed in to change notification settings - Fork 153
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
Syntax: improve elixirExUnitMacro #424
Conversation
Assumption: ExUnit macros are always followed by space. Technically, a macro's argument can be put into parentheses, but that happens only seldomly. It's more likely that a function gets named after an ExUnit macro. Fixes #418
(The CI passes. At first I just accidentally pushed the branch to this repo directly instead to my fork. That's the failing part.) |
@@ -160,7 +160,7 @@ syn match elixirExceptionDeclaration "[^[:space:];#<]\+" contained con | |||
syn match elixirCallbackDeclaration "[^[:space:];#<,()\[\]]\+" contained contains=elixirFunctionDeclaration skipwhite skipnl | |||
|
|||
" ExUnit | |||
syn match elixirExUnitMacro "\(^\s*\)\@<=\<\(test\|describe\|setup\|setup_all\|on_exit\|doctest\)\>" | |||
syn match elixirExUnitMacro "\(^\s*\)\@<=\<\(test\|describe\|setup\|setup_all\|on_exit\|doctest\)\> " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea would be to use negative lookaround for the :
character to make sure it's not an atom. I've done that frequently in the indent code (example: https://github.com/elixir-editors/vim-elixir/blob/master/autoload/elixir/indent.vim#L112). That would avoid the parenthesis change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that would leave us with setup()
beinging highlighted as macro.
Basically I try to distinguish between 3 cases: test ...
, test()
, and test:
. I know the issue is just about the test:
case, but I guess functions named test()
or setup()
are not too uncommon.
So.. assuming that the ExUnit macro is always followed by space, we can either add a space at the end of the regex or use that negative lookaround like \[:\(]\@!
. The outcome is the same.
I guess the question if we think that functions named test()
are more common than the ExUnit macro test
with parentheses. (I never saw the latter so far.)
Or we choose not to highlight ExUnit macros at all. That would avoid any tradeoffs. :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we choose not to highlight ExUnit macros at all. That would avoid any tradeoffs. :-P
Been thinking about this a lot lately - I'd definitely be in favor of a configurable list of macros (issue #432). Then we can get out of that business and everyone can just pick what they want 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I somehow missed that response. I agree! Closing this in favor of #432
Assumption: ExUnit macros are always followed by space.
Technically, a macro's argument can be put into parentheses, but that happens
only seldomly. It's more likely that a function gets named after an ExUnit
macro.
Fixes #418