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

support configurable list types #700

Merged
merged 2 commits into from
Feb 11, 2016
Merged

support configurable list types #700

merged 2 commits into from
Feb 11, 2016

Conversation

bhcleek
Copy link
Collaborator

@bhcleek bhcleek commented Jan 26, 2016

  • Prefer the quickfix window for commands whose output is considered to
    be errors for more than current buffer. e.g. :GoBuild.
  • Prefer the location list window for commands whose output is related
    specifically to the current buffer only (e.g: :GoFmt and
    :GoImplements).
  • Rename 'g:go_loclist_height' to 'g:go_list_height'.
  • Add a new option, 'g:go_list_type', to configure whether to always use
    the quickfix window, a location list window, or to vary based on the
    preferred window as documented above.

@fatih I'm not sure if you'd want to keep the default as I've coded it. The default here is to vary the list by the command, but that changes the 1.4 behavior, and we'd talked about preserving the 1.4 behavior by default.

I tried to update the relevant documentation where the location list was referenced to be accurate according to the preferred windows that I described above, but please double check my work.

Consider this a start point - I realize you may have lots of feedback; I'm happy to do any rework that you find necessary.

Fixes #696

@fatih
Copy link
Owner

fatih commented Feb 5, 2016

Thanks @bhcleek for the PR. I'm aware of this just hadn't the time to look at in more detail. This is a needed PR as it's really annoying to me too lately. So I want to use this to test how it works with quickfix + neovim. I'll let you know and also add comments.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Feb 7, 2016

Thank you for the update @fatih. I'm really looking forward to getting this merged; I've been using it since I submitted it and am pleased with the lists.

@fatih
Copy link
Owner

fatih commented Feb 7, 2016

@bhcleek could you rebase it again on top of master? Sorry for the inconvenience but seems like it's not mergable anymore due recent changes.

* Prefer the quickfix window for commands whose output is considered to
  be errors for more than current buffer. e.g. :GoBuild.

* Prefer the location list window for commands whose output is related
  specifically to the current buffer only (e.g: :GoFmt and
  :GoImplements).

* Rename 'g:go_loclist_height' to 'g:go_list_height'.

* Add a new option, 'g:go_list_type', to configure whether to always use
  the quickfix window, a location list window, or to vary based on the
  preferred window as documented above.
@bhcleek
Copy link
Collaborator Author

bhcleek commented Feb 7, 2016

@fatih I've rebased on top of master.

@fatih
Copy link
Owner

fatih commented Feb 8, 2016

Can you rename all a:quickfix variables to a:ltype (or a:listype) ? It's confusing as it's not clear what it is.

endfunction

function! go#list#Type(quickfix)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if we need this. It's yet another abstraction and seems like we don't need it. Can you remove it? Anywhere else just let use strings. Like:

if a:listype == "quickfix"
endif

This is more readable and we don't need to manually parse what 0 or 1 means.

@fatih
Copy link
Owner

fatih commented Feb 8, 2016

@bhcleek so as a final note, remove the Type() function and pass strings, such as "quickfix" or "locationlist" to each appropriate function.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Feb 8, 2016

There are two different times that resolution of the preferred window and g:go_list_type needs to happen; in the go#list functions themselves to decide which type of window to manipulate, and also in the callers of those functions, to decide which kind of command to run (e.g. lmake vs make). To satisfy that need, removing go#list#Type(), will require that the code that it contains be sprinkled in lots of places, because of the need to resolve the preferred window according to how the user has set g:go_list_type. With that in mind, do you still want go#list#Type() removed?

* Refactor to use strings for list types instead of an integer.

* rename variables that store the list type.
@bhcleek
Copy link
Collaborator Author

bhcleek commented Feb 8, 2016

@fatih I refactored to pass strings instead of integers. I'll wait to make the change to remove go#list#Type() until I get confirmation that you want it removed in light of my previous comment. Perhaps it would be better to rename it so its utility is clearer?

@fatih
Copy link
Owner

fatih commented Feb 8, 2016

@bhcleek sorry but I still didn't grasp the importance of go#list#Type(). It returns the passed string back to you. You have that information already? For example right now this is how the code is written:

    let l:listtype = go#list#Type(a:listtype)
    if l:listtype == "locationlist"
        return getloclist(0)
    else
        return getqflist()
    endif

But you can also write this as:

    if a:listtype == "locationlist"
        return getloclist(0)
    else
        return getqflist()
    endif

So what's the difference ?

@bhcleek
Copy link
Collaborator Author

bhcleek commented Feb 8, 2016

go#list#Type() doesn't just return what was passed to it. It returns the list type based on g:go_list_type. It returns what was passed in (e.g. a:listtype) only if g:go_list_type is not set to always use the quickfix or location list window.

Maybe this will help: https://github.com/bhcleek/vim-go/blob/master/doc/vim-go.txt#L1010

@fatih
Copy link
Owner

fatih commented Feb 9, 2016

Thanks @bhcleek, now it makes sense 👍 There is nothing that keeps me to merge this, I've already pulled and working with. Let me use this way 1,2 day and I'll merge it (so I can catch any errors meanwhile as I'm heavily using vim-go while developing our projects in my full time job). Thanks again 👍

fatih added a commit that referenced this pull request Feb 11, 2016
support configurable list types
@fatih fatih merged commit ff1e5f3 into fatih:master Feb 11, 2016
@fatih
Copy link
Owner

fatih commented Feb 11, 2016

Thanks @bhcleek a lot for your contribution. This is very useful and most needed feature we needed. If you see any errors or any improvements please feel free to open an issue or send a PR to discuss further.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Feb 11, 2016

I'm happy to help @fatih. If I find any problems, I'll submit a PR to get them fixed. FWIW, I've been using this for two weeks successfully.

👍

@svanharmelen
Copy link
Contributor

Really like this one 👍 Thx!

bhcleek added a commit to bhcleek/vim-go that referenced this pull request Mar 3, 2016
Fixes regression introduced in fatih#700, as documented on fatih#739 and fatih#743.
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.

quickfix vs location list
3 participants