mileszs / ack.vim

Vim plugin for the Perl module / CLI script 'ack'
Other
3.08k stars 396 forks source link

Fix :AckWindow for word-under-cursor searches #181

Open AndrewRadev opened 8 years ago

AndrewRadev commented 8 years ago

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.

ches commented 8 years ago

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:

AndrewRadev commented 8 years ago

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.