Improve connect behavior

[zhaocai]

1. connect to any Vim server and get the required vimrunner functions sourced.
2. open a new vim if the server is not available.

[zhaocai]

also add filename_escape to edit command!

[zhaocai]

failed rspec are related InvalidCommandError. I think redir command in vim just do not catch exception very well.

( Tested with MacVim )

Failures:

  1) Vimrunner::Client#command raises an error for a non-existent Vim command
     Failure/Error: expect {
       expected Vimrunner::InvalidCommandError but nothing was raised
     # ./spec/vimrunner/client_spec.rb:154:in `block (3 levels) in <module:Vimrunner>'

  2) Vimrunner::InvalidCommandError has a useful message
     Failure/Error: expect {
       expected Exception with message matching /Not an editor command: nonexistent/ but nothing was raised
     # ./spec/vimrunner/errors_spec.rb:24:in `block (2 levels) in <module:Vimrunner>'

[AndrewRadev]

I would rather not have connect spawn a new instance. It should be possible, even now, to do something like this:

# if you want to spawn a new server if it's not already there:
server = Server.new(:name => 'FOO')
if not server.connected?
  server.start
end

# if you want to wait until someone spawns the server:
server = Server.new(:name => 'FOO')
server.connect

Do you have a use case that this doesn't cover?

[zhaocai]

I have add the (:spawn => true) option to connect.

# if you want to spawn a new server if it's not already there:
client = Server.new(:name => 'FOO').connect(:spawn => true)

# if you want to wait until someone spawns the server:
client = Server.new(:name => 'FOO').connect

[AndrewRadev]

Sorry it took a while to get around to looking at this. The specs are probably failing because you don't have a recent enough version of Vim. Version 7.3 at patchlevel 860 (I think) should work, that patch fixes a bug with error handling with the --remote calls. The build is actually failing for a different reason, but don't worry about that. I have to get around and figure out how to fix them one of these days.

In any case, thank you for your work :). You found quite a few bugs I'd totally missed. I left you a few comments on the code, could you take a look at them before I merge it in? Some things I can adjust after merging, like coding style and documentation, but the :spawn option is something I'd rather not merge in, since I don't think connect should spawn.

[AndrewRadev]

These should be @r and @w. Though, I'd rather remove this change instead, since I'd prefer connect not to spawn a new instance.

[zhaocai]

I use macvim, the newest vim has
Included patches: 1-806

[zhaocai]

about the :spawn option, I think it make the code more concise. Compare

server = Server.new(:name => 'FOO')
if not server.connected?
  server.start
end

with

client = Server.new(:name => 'FOO').connect(:spawn => true)

And the meaning for the option is clear. Anyway, it is up to you to decide. Not a big issue to me.

[AndrewRadev]

It's true that it's shorter, but to me, this makes the connect method do too many different things. For example, with this we'd have two equivalent ways of spawning a server:

Server.new(:name => 'FOO').start
Server.new(:name => 'FOO').connect(:spawn => true)

So you can kind of use connect for different things and this makes the API a bit more confusing to me. In a lot of ways, I guess it's a matter of style, but since you leave the decision to me, for now, I'll choose to keep the :spawn option out of the API.

Everything else currently looks good to me, so after you remove it, I'll merge the PR.

[zhaocai]

# if you want to spawn a new server if it's not already there:
server = Server.new(:name => 'FOO')
if not server.connected?
  server.start
end

still, the meaning of the code above does not intuitively feel right. I have not called connect, why should I check for connected?

[AndrewRadev]

That's a very good point. Now that I see the code example, it looks strange to me, too, but I think this is an issue of naming. I think a better name for connected? is running?, because it simply checks if there's a running Vim server, regardless of whether it's been connected to or started manually. The code would then look like this:

# if you want to spawn a new server if it's not already there:
server = Server.new(:name => 'FOO')
if not server.running?
  client = server.start
end

# if you want to connect to a running Vim instance:
server = Server.new(:name => 'FOO')
if server.running?
  client = server.connect
else
  raise "The server FOO is not started!"
end

# or, of course, in this case you can just try connect, which would block
server = Server.new(:name => 'FOO')
client = server.connect

What do you think of this naming?

[zhaocai]

running is a better name. I have finished updating the code. However, it makes me feel awkward to lay burden to the API caller. It becomes the caller's duty to check and make sure the server is running and connected. An one liner become 7 lines.

compare:

    def connect(options= {})
      server = Vimrunner::Server.new(options)
      if not server.running?
        client = server.start
      else 
        client = server.connect
      end
      return client
    end

   def connect(options = {} )
     Vimrunner::Server.new(options).connect(:spawn => true)
  end

Suppose you are not the maintainer of the source code but a user. You just want to connect to a vim server and tell vim the execute Gdiff, for example, on a file. You do not want to pay too much attention on if the vim is started or not. you just want to tell the API to connect it (spawn if necessary).

sorry, originally I thought it is not a big deal about the :spawn option. Now I have spent some time to think about it. I really feel it is the right way to go.

In short, I think the API should reduce but introduce burden (examining all kinds of possible situations) to the caller.

[AndrewRadev]

I wouldn't say "all kinds of possible situations" should be expected, but what you describe sounds like a useful enough use case that I would agree with. If it's come to this, though, I'd like to brainstorm some ideas on how to achieve this. @mudge, if you have thoughts on the matter, feel free to weigh in, I'd appreciate your feedback.

First is Option 1, your initial implementation. The connect method gets a :spawn => true option that optionally calls start:

client = Server.new(:name => 'FOO').connect(:spawn => true)

As I mentioned, I don't like this approach much. It puts too much functionality in connect, and it doesn't seem like it has any benefits over, say, start(:connect => true). Moreover, it seems backwards -- connect should block and wait for a Vim if necessary, but if this option is given, it shouldn't. One different option I've been thinking of putting in connect is :timeout => <seconds> in order to control how long to wait until giving up. However, what would the code do with :spawn => true, :timeout => 3? Should it ignore the timeout parameter completely or maybe wait for 3 seconds before spawning a new server? I think this makes the logic a bit convoluted and the implementation complicated.

The idea for start(:connect => true), Option 2, is actually one I like more. Another name for the option I'm considering is :reuse => true, since it's a hint to Vimrunner to try and reuse a server if it finds it.

client = Server.new(:name => 'FOO').start(:reuse => true)

The connect call will retain its blocking behaviour. I still feel uncomfortable with this, simply because additional options to start may conflict with :connect => true the same way as I described above. For instance, the :foreground flag seems like it should go on start, not on the constructor, since if a connect is invoked, there's no way to control whether the Vim instance was foregrounded or not. But this flag is something I'll think about and I did make a separate issue for it and any related discussion that may pop up.

To me, the common thread is that mixing up connect and start may get weird if they have optional arguments, especially if those get more and more. There's no guarantee this would happen, but it could, and the core issue is that the two methods have two different ways of dealing with a Vim instance.

A simple Option 3 would be to just reimplement this helper method you demonstrated in the actual library, and call it connect_or_start.

class Server
  def connect_or_start
    if running?
      connect
    else
      start
    end
  end
end

client = Server.new(:name => 'FOO').connect_or_start

This has the drawback that you wouldn't really give it options, but then again, you shouldn't -- it's simply a shorthand method to optimize a use case. If you need more complicated calls, you could just implement the logic yourself. I'm not sure how much this is acceptable, though -- neither start, nor connect currently have any options, and it's hard to determine how common it would be for a user to need additional options to one or the other. Of course, we could just add options on connect_or_start and dispatch them either to connect, or to start, but then we get a similar option mix-up as I described above.

The last option I can think of at this time, Option 4 is to simply slice them up a bit more. Make connect return nil instead of timing out and have the user check for a connection themselves.

# If you want to get the current `connect` behaviour
server = Server.new(:name => 'FOO')
server.wait_until_running
client = server.connect

# If you want to get the current `start` behaviour
server = Server.new(:name => 'FOO')
client = server.start

# If you want to get the new "connect if possible, else start"
server = Server.new(:name => 'FOO')
client = server.connect || server.start

Not everything is a one-liner, but it's still quite concise and all of wait_until_running, connect, start can have additional options that further define their behavour in the future. There could also be a connect which returns nil and a connect! which blocks and explodes, and maybe wait_until_running and wait_until_running! with the same behaviour, though I'm not sure about that.

What do you think? Do you still prefer your original implementation, or does one of the other ones look better to you? Any different ideas?

[zhaocai]

First of all, a few things I agree on:

• :foreground flag seems like it should go on start
• :timeout => in order to control how long to wait until giving up.

options

option 2 , start(:connect => true) and start(:reuse => true):

I inclined to disagree with this one because start implicates :connect (not just the meaning, this is also in the source code). And :reuse seems to be the only option - no need to make an option for the only option.

The reason you disagree on option 1 is that "it puts too much functionality in connect". But I do not think putting too much functionality on :start can be better idea. :start should do what it means - start the vim server. If the vim server is started already, then there is nothing for :start to do.

option 3: connect_or_start

I do not know what to say. I am not against it. And I do not prefer it.

option 4:

I think we should not change the behavior of connect. It should raise exception instead of return nil. Otherwise, the caller has to make extra effort to check for nil. And the caller can not simply put the connect call in a try block.

option 1:

About :spawn => true, :timeout => 3, I have implemented this as:

      if !connected? && options[:spawn]
        @r, @w, @pid = spawn
      end
      wait_until_started

In Summary

I think we should set up common rules or goals for this discussion. For me, my rule of thumb here is to present clear and intuitively correct meaning for the API interface. And I still feel option 1 is better because it has clear meaning for what it does.

And I do not see why it is wrong to give connect more functionality if it does what it does: "you ask to connect to the vim server. OK, I will do it. And if the vim server is not started, I will start it for you."

[AndrewRadev]

And I do not see why it is wrong to give connect more functionality if it
does what it does: "you ask to connect to the vim server. OK, I will do it.
And if the vim server is not started, I will start it for you."

Yes, but that is two things. The fact that there's an "and" in there hints at this as well. Here's something else I realized -- Most of the initialization options actually belong to start. The connect call has no use of :executable, :foreground, :vimrc, it only needs a name. So start might look like this now:

client = Server.new(:name => 'FOO').start({
  :executable => '/opt/bin/gvim',
  :foreground => false,
  :vimrc      => 'my_custom_vimrc'
})

So now, if you call connect(:start => true), how does it call start, what options does it provide? If it just calls start with the defaults, then the moment you need to change something (like GUI vs headless), you need to rewrite the thing not to use connect but to check for running? and then start. Alternatively, we could provide the options like this:

client = Server.new(:name => 'FOO').connect(:timeout => 3, :start => {
  :executable => '/opt/bin/gvim',
  :foreground => false,
  :vimrc      => 'my_custom_vimrc'
})

But this is way too many options on a single method, and they change its behavior dramatically. I don't want to burden a single API call with so many responsibilities. There's no need for it considering you can easily make the check yourself. Compare with option 4:

server = Server.new(:name => 'FOO')
client = server.connect(:timeout => 3)

if not client
  puts "Couldn't connect to an existing Vim, starting server"
  client = server.start({
    :executable => '/opt/bin/gvim',
    :foreground => false,
    :vimrc      => 'my_custom_vimrc'
  })
end

This is not more concise, but it's also not much more verbose. It gives you more flexibility over the decisions -- notice the puts in between, indicating what exactly happened in the process. More importantly, the two methods connect and start do completely separate things and don't have any public connection to each other. Whether or not they share code underneath is an implementation detail.

As a side note, I'm starting to think that there might be a need for a different server, something like an ExternalServer, since most of the options on initialization of Server seem to be completely useless to the connect call and they might even be misleading -- you might provide a gvim executable as an option, but get connected to a headless vim. But this is something that needs to be thought about, it may not work out very well. It's also an implementation detail, so it's not important for this discussion.

It should raise exception instead of return nil. Otherwise, the caller has to
make extra effort to check for nil. And the caller can not simply put the
connect call in a try block

I don't think that checking for nil would be much more effort than a begin-rescue block, but it's not a good idea to use exception handling for control flow anyway. As I mentioned, I see this with two methods:

server = Server.new(:name => 'FOO')

# If you want to just try connecting to a Vim and it may or may not work, use
# `connect`:
client = server.connect
if not client
  puts "Couldn't connect to server 'FOO', doing something else..."
  # ....
end

# If you expect to be able to connect to a Vim with the given name, but for
# some reason you fail, then you should just raise by using `connect!` -- it's
# an exceptional situation:
client = server.connect!
client.do_stuff

On my side, I'm starting to like option 4 more and more. The running? call could stay, but it won't be necessary in this use case and the simplest case could easily be covered by server.start || server.connect, which is quite short and readable.

As I thought about this, I also remembered the conventions we currently have in place. We have Vimrunner.start, Vimrunner.start_gvim and Vimrunner.connect as simple shorthands for more complicated expressions. If you don't need to deal with servers, clients and whatnot, this is all you need to do to get up and running. If you do need something more complicated, then you should instantiate a Server instance and get a client out of it, but this means that you may have to write some additional code like if-clauses, checks and so on. At this level of abstraction, it's impossible to cover all possible cases -- the best we can do to provide a good user experience is to provide simple, focused classes and methods, each of which has a single well-defined purpose. This would let users combine them in whatever ways fit their workflow, without imposing any design decisions on them. I think option 4 satisfies that nicely, though I may have to write some tests and think about it some more, especially the distribution of options. I may be missing some problem with it, and I need to see how it works out with the current use cases.

Also, I don't think this is a good place for this discussion anymore. I'd like to merge your code in as it is right now, with your implementation of :connect => true and then open a new issue to decide whether to keep it like this or to change it. Are you okay with that?

[zhaocai]

Please go ahead. After your explanation, I am 90% agree with you now. let me know when you finalize the code.

[mudge]

I agree with the principle of providing simple, composable abstractions and then allowing people to build their own abstraction layers on top.

In this case, the fact that connect and start can be combined as primitives by a client into a more generic "connect, starting if need be" would suggest that connect and start are good starting points and we shouldn't complicate them too much to muddy that.

I think this is something we're still trying to figure out for Vimrunner: what primitives should it expose and what is its primary use case. Is it an abstraction on top of Vim itself or just an abstraction over remote-controlling Vim (which is a little more specific)?

[AndrewRadev]

Merged, thank you for your work.

I've created a new issue to continue the discussion, #22. I won't release a new version, since :start => true may be removed and there's no point in releasing two successive versions with opposite API changes, even if we're at an unstable version 0.x. However, I will try to finalize things this weekend and release.

[AndrewRadev]

I think this is something we're still trying to figure out for Vimrunner:
what primitives should it expose and what is its primary use case. Is it an
abstraction on top of Vim itself or just an abstraction over
remote-controlling Vim (which is a little more specific)?

I guess I can't really say. Vimrunner has a lot of convenience over the raw --remote calls, but I don't know what level of abstraction that makes it. Up till now, the "primary use case" has been testing, I think, but I can see how it can be useful for other things as well. After this discussion, I've started to consider playing around with this some more, for instance. Let's wait and see, I suppose :).

[AndrewRadev]

A new version has been pushed to rubygems, 0.3.0.