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

re-evaluate sameids after a rename #975

Closed
wants to merge 1 commit into from

Conversation

bhcleek
Copy link
Collaborator

@bhcleek bhcleek commented Jul 30, 2016

Re-evaluate sameids after a rename. This is especially useful when go:go_auto_sameids is not set.

@fatih
Copy link
Owner

fatih commented Jul 31, 2016

Hi @bhcleek. To be honest I didn't like this. I don't want that a feature is called because of other commands. If you really want this you should create custom vim function in your vimrc and use that. Otherwise this causes side effects. Every single command should just do what it's meant for. In long term code like this is not maintainable either. Feel free to reopen if you think we should not close this. Thanks.

@fatih fatih closed this Jul 31, 2016
@bhcleek
Copy link
Collaborator Author

bhcleek commented Jul 31, 2016

@fatih I think you may have misunderstood what this change does. It doesn't unconditionally highlight the identifier under the cursor after a rename; it only highlights the identifier under the cursor if it was already highlighted before the rename. Perhaps a demonstration will help:

master currently does this:
master

With this patch, the behavior is:
sameid-and-rename

The behavior on master seems wrong to me. The user has highlighted the identifier under the cursor and then renamed it. After renaming, I would expect the identifier under the cursor to be highlighted, but on master the length of the previous name of the identifier under the cursor has determined the highlight length.

@fatih fatih reopened this Aug 8, 2016
@fatih
Copy link
Owner

fatih commented Aug 8, 2016

@bhcleek upon rethinking I think you are right. We can make vim-go a little bit smarter. Even though I believe it's the user's responsibility to clear before renaming, I assume there will be a lot of people doing this. It can't merged though, can you please rebase against master again ?

@bhcleek bhcleek force-pushed the sameid-and-rename branch from caeddbb to 5aa1e3d Compare August 8, 2016 03:40
@bhcleek
Copy link
Collaborator Author

bhcleek commented Aug 8, 2016

PTAL

I'm not entirely sure the technique I've employed here is the right one. autoload/go/coverage.vim demonstrates another technique that may be preferable: hooking into the autocmd events for the current window. What would you think about doing something more like what is employed there? That would put all the logic for dealing with the sameid in autoload/go/guru.vim instead of having something special over in autoload/go/rename.vim.

@@ -72,6 +72,8 @@ function! go#rename#Rename(bang, ...)
" we need a way to get the list of changes from gorename upon an success
" change.
silent execute ":e"

call go#guru#SameIdRenamed()
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should do it different. There should be a function called go#guru#HasSameId() which returns a true if it's been highlighted. And then you call it like this:

if go#guru#HasSameId()
  " Call sameids again to redraw the highlighting  
  call go#guru#SameIds(-1)
endif

Does this makes sense? I didn't like the SameIdRenamed() function so I would be happy if you can remove it.

@fatih
Copy link
Owner

fatih commented Aug 8, 2016

What would you think about doing something more like what is employed there?

@bhcleek what do you have in mind? If you can show me a working example (maybe a different PR) I can take a look.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Aug 9, 2016

After working up the other approach, I definitely like it better. It's a cleaner implementation and ensures the highlighting is applied correctly in a greater range of scenarios. I'm closing this in favor of #998.

@bhcleek bhcleek closed this Aug 9, 2016
@bhcleek bhcleek deleted the sameid-and-rename branch August 9, 2016 04:44
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