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

quickfix vs location list #696

Closed
bhcleek opened this issue Jan 24, 2016 · 10 comments · Fixed by #700
Closed

quickfix vs location list #696

bhcleek opened this issue Jan 24, 2016 · 10 comments · Fixed by #700

Comments

@bhcleek
Copy link
Collaborator

bhcleek commented Jan 24, 2016

The most recent release, 1.4, changed lists to use the location list instead of the quickfix list. This is a useful improvement for functions that operate on a single buffer (e.g. GoCallers). But for functions that operate on a package instead of the buffer (e.g GoBuild), I strongly prefer the quickfix list, (e.g. GoBuild).

I'm not certain, but I think the difference in my mind is that the location list is most appropriate for lists of locations that are the result of a search (e.g. GoImplements), while the quickfix list is appropriate for lists of locations that communicate needed user action (e.g. GoTest).

I've read your comments on #626. Would you consider a pull request that introduces a new option to allow the quickfix list to be used for functions that generate a list of errors that require action if the new option's default is to preserve the 1.4 behavior?

@fatih
Copy link
Owner

fatih commented Jan 24, 2016

Hi @bhcleek I would be happy to merge it. I was thinking of having a switch, that changes the functions in list.vim to use quickfix instead of location list. I've already refactored and laid out the foundation so it's pretty easy! The only change we need is to be inside list.vim file, as every other function uses those functions. By default it should be locationlist and an option g:go_list_type="locationlist" or g:go_list_type="quickfix" should change it. What do you think?

Or we don't provide an option (it's going to be hidden) and the list.vim would accept a list_type string variable, indicating which type of list it would use. So we would have our own set of usages and would enforce it to the user. Though I'm not sure if that is the correct way, maybe it its.

@svanharmelen
Copy link
Contributor

The first option (having a g:go_list_type option) would make me very happy 😉

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jan 24, 2016

@fatih, I would be happy with either of your proposals. A strong argument can be made that a combination of the two proposals is the correct thing to do: some commands unconditionally use the location list, and other commands populate one list or the other based on a switch.

Before we decide, though, can you expand on the second proposal? Wouldn't using the quickfix unconditionally be subject to the race condition in NeoVim that you mentioned in #626?

@fatih
Copy link
Owner

fatih commented Jan 24, 2016

Yes, the reason my second proposal was to avoid these kind of things. For example even though we would use quickfix for :GoBuild, we could use locationlist if it's being used under NeoVim. That would very easy to do if the functions accepts a first parameter, list_type, which could change the way a list is created/displayed, etc..

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jan 24, 2016

Thank you for the clarification. I prefer the second option.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jan 25, 2016

Can the two approaches be combined?

I think this could be phased relatively simply: introduce an option now that supports three values: always use the location list, always use the global list, and vary the lists by command. Simultaneously, introduce a new parameter to the list.vim functions that specifies the list type. The final step, then, is to modify list.vim to use quickfix when the list type is errors, the location list when the list type is locations, and of course, ignore the type list type if the option says to globally use one list or the other.

I'm happy to help with this in any way if you want.

@fatih
Copy link
Owner

fatih commented Jan 25, 2016

That sounds a good plan to attack the problem 👍 . Just the type names should be consistent, such as quickfix, locationlist, etc.. We can check the vim docs to get the correct naming. A quickfix can be used for all kind of features, it doesn't need to be errors.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jan 25, 2016

I agree fully on the consistency; I only threw out some proposed names to give us a starting point of discussion. The details always come later 👍

@svanharmelen
Copy link
Contributor

After reading you comments, I guess the second option would make me just a happy 👍

@bhcleek
Copy link
Collaborator Author

bhcleek commented Jan 25, 2016

I have a branch that I believe is just about ready. I'll test it tomorrow, resolve any problems I find, and then submit a PR.

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 a pull request may close this issue.

3 participants