AndrewRadev / vimrunner

Control a vim instance through ruby code
MIT License
238 stars 13 forks source link

Upgrade to use rspec 3 and improve code structure #39

Closed agilecreativity closed 9 years ago

agilecreativity commented 10 years ago

Hi Andrew,

Once again thanks for writing a great gem.

Here are the quick summary of the PR:

1st commit - Update gemspec file

Please ignore/delete my other PR (eg https://github.com/AndrewRadev/vimrunner/pull/37) and use this one instead as it contain smaller commits that you can easily review before merge.

If you do need me to make any adjustment, please let me know.

@mudge I have included some of your idea to this one as well.

Best, Burin

AndrewRadev commented 10 years ago

I merged @mudge's pull request first, since it was easier to pull off. I think when he suggested you split the PR into different parts, he meant different branches, with different pull requests. We can probably discuss here as well, though. We may have to cherry-pick commits.

First, the rspec upgrade was done by @mudge, and I think we can leave this aside. Then there's the debugging gems and .ruby-version ignore, which I'm okay with.

As for the require-s, I don't really have a strong opinion either way. On one hand, having the requires in each file shows their dependencies. On the other hand, putting all dependencies in a single file has the benefit that they're all in one place and the classes themselves are kept "clean" from that knowledge. I usually lean towards the first option. What do you think on the matter, @mudge?

I'm also undecided on lib/vimrunner/vimrunner.rb. Based on the filename, I'd expect it to contain a class called Vimrunner::Vimrunner. I do seem to remember gems that use this pattern though (putting the top-level module in a <gemname>/<gemname>.rb file). It still seems a bit more consistent to have lib/vimrunner.rb host the toplevel module. Again, what are your thoughts on the matter, @mudge?

mudge commented 10 years ago

Re the requires, I prefer that they stay in the files that actually use them: they are a form of documentation after all and should explicitly state the dependencies of a particular piece of code.

Re lib/vimrunner/vimrunner.rb, I agree with @AndrewRadev that I would expect that to contain Vimrunner::Vimrunner as would the autoloading mechanism in Rails. As we tell people to require vimrunner then -- practically speaking -- we can control exactly which files are loaded but I'd like to follow convention so that autoloading works predictably.

agilecreativity commented 10 years ago

Hi @mudge and @AndrewRadev,

I am quite happy for the changes you have already merge for other PR. Please feel free to close this one.

Thanks, Burin

AndrewRadev commented 9 years ago

I was considering looking at the other additions, but it seems like it's mostly debugging tools. While they might be useful, I'd say they can be added on-demand, so I'll go ahead and close this.

Thanks for the help, @agilecreativity.