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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+12 −1
Closed

Syntax: improve elixirExUnitMacro #424

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions spec/syntax/exunit_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -171,4 +171,15 @@ module MyTest do
end
EOF
end

it 'does not highlight keys named after macros' do
expect('test: foo').not_to include_elixir_syntax('elixirExUnitMacro', 'test')
end

it 'does not highlight functions named after macros' do
# Tradeoff: 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.
expect('setup(foo)').not_to include_elixir_syntax('elixirExUnitMacro', 'setup')
end
end
2 changes: 1 addition & 1 deletion syntax/elixir.vim
View file
Original file line number Diff line number Diff line change
@@ -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\)\> "
Copy link
Collaborator

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

Copy link
Contributor Author

@mhinz mhinz Apr 21, 2018 •
edited
Loading

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

Copy link
Collaborator

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 😄

Copy link
Contributor Author

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

syn match elixirExUnitAssert "\(^\s*\)\@<=\<\(assert\|assert_in_delta\|assert_raise\|assert_receive\|assert_received\|catch_error\)\>"
syn match elixirExUnitAssert "\(^\s*\)\@<=\<\(catch_exit\|catch_throw\|flunk\|refute\|refute_in_delta\|refute_receive\|refute_received\)\>"