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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AndrewRadev
Copy link

As discussed in #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.

If an argument was not given, the command didn't take the word under the
cursor, since the locations that were given to the ack#Ack() function
worked as an "args" argument. Separating "args" and "locations" fixes
the issue.
@AndrewRadev AndrewRadev changed the title Fix :AckWindow for word-under-cursor searches Fix :AckWindow for word-under-cursor searches Apr 30, 2016
@ches
Copy link
Collaborator

ches commented May 3, 2016

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?

@AndrewRadev
Copy link
Author

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.

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.

None yet

2 participants