ack.vim
ack.vim copied to clipboard
Fix :AckWindow for word-under-cursor searches
As discussed in https://github.com/mileszs/ack.vim/pull/171, this PR only fixes a bug with :AckWindow
and :AckHelp
.
Basically, if an argument was not given, the command doesn't take the word under the cursor, since the locations that are given to the ack#Ack()
function work as an "args" argument. Separating "args" and "locations" fixes the issue.
Since locations are only used internally (if a user enters locations, they'll just be stuck in the catch-all args
argument), it makes more sense for them to be a list.
A different way of handling this would be to provide a dict with "options" or something, ruby-style. That way, any additional params that need to be communicated between functions privately would not need to change the signature.
Or, argument handling could somehow be extracted to a separate function, so every one of ack#AckWindow
, ack#Ack
and so on invokes it first, something like:
let [args, other_stuff] = s:ParseArgs(a:args, a:count, a:etc_etc)
if args == ''
return
endif
Or something. There would still be some extra work, since ack#AckWindow
calls ack#Ack
, so both would have to do argument parsing, so I don't know if it's a good approach.
Anyway, I'd rather just go for this solution, since I think it's simple enough and doesn't seem to have a lot of drawbacks. But let me know what you think.
This is great, thanks for the fix(es) and thinking through the proposal of an alternative for handling arguments. To me, it already helps the clarity of intent a great deal to pass around an array rather than string as in #171.
Interestingly, by the way, this change to GetDocLocations
without the glob
is not exhibiting the Vim pager mode intrusion for me with Dispatch & tmux the way #182 does…
One request, and one question:
- While we're doing this, let's change the naming to "paths" instead of "locations" throughout—that includes
GetDocLocations
, it's a private function anyway so we can change it freely. To me, "location" is more ambiguous whether it could refer to a file/path or a cursor position, and that could become more confusing when we get back to the visual selection support. At least to me, "path" quite clearly suggests either a directory or file. - Now, about the parameters, it's too bad VimL doesn't support default values or this could be quite clean. An alternative we could try though for approximating that is adding a vararg. That gets a bit messier internally, I agree, but maybe less so than an explicit/required "options" dict and on the plus side it wouldn't break backwards compatibility of
ack#Ack
by requiring a new argument—it is a "public" function that people could possibly be calling with custom scripts. What do you think?
To me, "location" is more ambiguous whether it could refer to a file/path or a cursor position
Yeah, you're right, I hadn't thought of that. I've renamed it.
Now, about the parameters, it's too bad VimL doesn't support default values or this could be quite clean. An alternative we could try though for approximating that is adding a vararg. That gets a bit messier internally, I agree, but maybe less so than an explicit/required "options" dict and on the plus side it wouldn't break backwards compatibility of ack#Ack by requiring a new argument—it is a "public" function that people could possibly be calling with custom scripts. What do you think?
Adding a vararg could work, especially if that vararg is that extra options dict. That way it would be possible to make further changes without much issue. Another thing I'm thinking of, though, is why not just create a private, script-local function that does the heavy lifting? That way the ack#Ack
function's interface remains the same and it just changes into:
function! ack#Ack(cmd, args)
return s:Ack(a:cmd, a:args, [])
endfunction
In any case, it's your call, so pick one option and I'll implement the change. I'm fine with either one, I think.