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

Make go_list_type configurable for each command independently #1408

Closed
fatih opened this issue Aug 13, 2017 · 4 comments · Fixed by #1415
Closed

Make go_list_type configurable for each command independently #1408

fatih opened this issue Aug 13, 2017 · 4 comments · Fixed by #1415

Comments

@fatih
Copy link
Owner

fatih commented Aug 13, 2017

Currently this setting is global. However we could make it better by giving the user the ability what they want to use for each command. So instead of

let g:go_list_type = "locationlist"

we could have:

let g:go_list_type_commands = {
  "GoFmt": "locationlist",
  "GoBuild": "quickfix"
}

This would override the command's default setting for the list.

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 13, 2017

(partly cross posted from #1365)
I like the idea of making the list type configurable for each command.

Having said that, though, the defaults for each command should still be consistent. In light of #1365, what should be the criteria for deciding the default list type for each command? Making GoFmt use the quickfix list deviates from the original criteria we used; I wonder if there's an implication here that other commands that currently use the location list should instead be using the quickfix list: if GoFmt's default wasn't correct, then are there other commands that are also using the wrong list type for their default? How can we consistently evaluate that?

For reference, #700 introduced the use of both list types and was completed to fix #696.

Use cases are always helpful, and here's one that seems to be a counter point to #1365:
I thought of a use case where using the quickfix list for GoFmt may be less than ideal for most users:

  1. :GoBuild
  2. Many errors across several files are shown in the quickfix list
  3. :cfirst to go to the first error
  4. edit to fix error, but introduce a problem for GoFmt
  5. :w (assuming g:go_fmt_autosave is set)
  6. quickfix window that contained compiler errors is now replaced with errors from GoFmt

As a user, I would find that surprising. What I'd want to happen is to be able to correct the formatting problem and then do :cn to go to the next compiler error, but now I have to do :GoBuild instead to repopulate the quickfix list with compiler errors.

@fatih
Copy link
Owner Author

fatih commented Aug 15, 2017

We can go back for gofmt for location list. I think we just need a comment so next time I don't accidentally change it. If you want to work on it feel free to start. I know that you already worked on this behavior.

So I think the following should apply in order:

  1. A default for all commands individually, locationlist for :GoFmt, quickfix for :GoBuild etc...
  2. A setting to change all settings globally (so for example someone just can use quickfix).
  3. A setting that overrides individual commands (this issue).

Do you think this is clear and is worth tackling ?

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 15, 2017

SGTM

I've already started the work and should be able to finish it up soon. I was considering keeping just the single configuration value, g:go_list_type and allowing it to support both maps and a string. If it's a string, then it would apply to all commands, but if it's a map, then it would each command be specified to deviate from that command's default. What do you think?

@fatih
Copy link
Owner Author

fatih commented Aug 15, 2017

I'm usually against dynamic types as it's make it hard to refactor :/ I know that Vim itself encourages it in some places. I think having g:go_list_type just for global setting for all commands and introduce a new g:go_list_type_commands would be better.

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.

2 participants