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

Highlighting arguments #1587

Merged
merged 7 commits into from
Dec 7, 2017
Merged

Highlighting arguments #1587

merged 7 commits into from
Dec 7, 2017

Conversation

igrmk
Copy link
Contributor

@igrmk igrmk commented Nov 28, 2017

I prefer to highlight function argument names to be able to distinguish them from their types.
This pull request:

  • enables you to highlight them as goArgumentName,
  • fixes simple typo,
  • fixes receiver regular expression and let you use receiver type only without name e.g. for functions returning constant,
  • highlights receiver name as goArgumentName as well because it is an argument after all.

It deliberately doesn't work for nested functions. It can be a case when a function is an argument type or it is inside function declaration (it can be a field of anonymous struct for example). Unfortunately I didn't find a way to support it without big rewrite of syntax rules. So it is checked if declaration contains inner parentheses and this highlighting is turned off if so.

Before:
before

After:
after

It relates to this pull request #1580 that I closed because it doesn't work well with nested functions. This one has additional fixes as well.

@arp242
Copy link
Contributor

arp242 commented Nov 28, 2017

Seems to work well in a quick test. Thank you! The only thing that doesn't seem to work is return values for methods:

2017-11-28-192716_591x89_scrot

I would expect s and err to be red there, too?

I think it would be better to add a new option for this, for example g:go_highlight_function_parameters, especially since it doesn't actually do anything by default. So people that never do :hi goArgumentName [..] will see no changes, but still pay the performance overhead of all the regexps you've added.

If you hide it behind a flag we could also highlight it by default. Maybe :hi link goArgumentName Identifier?

@igrmk
Copy link
Contributor Author

igrmk commented Nov 29, 2017

Thank you. Yep, I agree with all these issues. I'll implement them soon.

@igrmk
Copy link
Contributor Author

igrmk commented Nov 29, 2017

Done. Only I named option g:go_highlight_function_arguments. I suppose argument is more correct word.

syntax/go.vim Outdated
@@ -350,12 +354,15 @@ if g:go_highlight_functions != 0
syn match goPointerOperator /\*/ nextgroup=goReceiverType contained skipwhite skipnl
syn match goFunction /\w\+/ nextgroup=goSimpleArguments contained skipwhite skipnl
syn match goReceiverType /\w\+/ contained
syn match goSimpleArguments /(\(\w\|\_s\|[*\.\[\],\{\}<>-]\)\+)/ contained contains=goArgumentName nextgroup=goSimpleArguments skipwhite skipnl
if g:go_highlight_function_arguments != 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't look too deeply yet, but would it be possible to move this out of the if g:go_highlight_functions != 0 block? Then people can set these two settings independently.

If this isn't possible then that's not a disaster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to parse "func", probably receiver, and function name before trying to highlight arguments. So it is possible but we will need to copy half of just function rules. So I put these new rules inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However it may be worth to enable g:go_highlight_functions if this new setting is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it and pushed. Otherwise users can spend their time figuring out why it doesn't work although setting is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

However it may be worth to enable g:go_highlight_functions if this new setting is enabled?

Sounds good 👍

@arp242 arp242 force-pushed the highlighting-arguments branch from 1de3f47 to 545a645 Compare December 7, 2017 17:35
@arp242
Copy link
Contributor

arp242 commented Dec 7, 2017

Works well in my testing. Thanks!

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.

2 participants