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

check group in when toggling same ids #1538

Closed
wants to merge 3 commits into from

Conversation

tmatias
Copy link
Contributor

@tmatias tmatias commented Oct 28, 2017

Currently, ToggleSameIds doesn't check the group of the match and, as I set up vim to highlight characters that go beyond column 80 (just find it a mostly helpful heuristic), when I open a file that contains longer lines, ToggleSameIds always try to clear highlighted ids, because getmatches() return all highlights and not just the GoSameId ones.

So, as already done in ClearSameIds, I think we should also test the group here.

@bhcleek bhcleek self-requested a review October 28, 2017 17:06
@bhcleek
Copy link
Collaborator

bhcleek commented Oct 28, 2017

Thank you for the contribution.

go#guru#ClearSameIds, which is called by go#guru#ToggleSameIds, already checks the group and only clears matches that are in the goSameId group. See https://github.com/fatih/vim-go/blob/master/autoload/go/guru.vim#L537-L542.

@bhcleek bhcleek closed this Oct 28, 2017
@tmatias
Copy link
Contributor Author

tmatias commented Oct 28, 2017

@bhcleek you are correct about that.

However, ToggleSameIds itself does not and so, if there are any matches, even if not in the goSameId group, it will always decide that it should call ClearSameIds and therefore the toggling won't work as expected.

(see PR description :P)

@bhcleek bhcleek reopened this Oct 28, 2017
@bhcleek
Copy link
Collaborator

bhcleek commented Oct 28, 2017

I see. In that case, instead of repeating the logic for iterating through the matches, go#guru#ClearSameIds should return a non-zero value when no matches were found and that value should be checked in go#guru#ToggleSameIds, which should call go#guru#SameIds if no goSameId groups where cleared. Will you refactor to do that?

@tmatias
Copy link
Contributor Author

tmatias commented Oct 28, 2017

That sounds like a better approach. Thank you.

I'll do that.

  so we don't need to replicate its group check logic inside ToogleSameIds
@tmatias
Copy link
Contributor Author

tmatias commented Oct 28, 2017

Returned a positive value for success and zero for the other case, but I can change that to return a negative value if it would make things better. (Not knowing much vimscript, it just seemed nicer.)

Copy link
Collaborator

@bhcleek bhcleek left a comment

Choose a reason for hiding this comment

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

Yes, please invert the return value; conventionally 0 is success, and non-zero is failure. Since go#guru#ClearSameIds is supposed to delete any goSameId groups, failure should mean that none were removed.

let m = getmatches()
for item in m
if item['group'] == 'goSameId'
call matchdelete(item['id'])
let l:cleared = 1
endif
endfor

Copy link
Collaborator

Choose a reason for hiding this comment

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

If no matches were in the goSameId group, then return early here so that the BufWinEnter events won't be removed.

@bhcleek
Copy link
Collaborator

bhcleek commented Oct 28, 2017

Thank you!

@bhcleek
Copy link
Collaborator

bhcleek commented Oct 29, 2017

Thank you for contributing. I squashed your commits and merged them into master.

@bhcleek bhcleek closed this Oct 29, 2017
@bhcleek bhcleek reopened this Oct 29, 2017
@bhcleek bhcleek closed this Oct 29, 2017
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