Mon-Ouie
commented
10 years ago Hey,
There are some problems with this patch. First of all, all of the files should be in a pry-remote directory. Otherwise require 'client' could cause the pry-remote gem to be activated.
Also even though Ruby 1.8 is dead, I feel like there's no point in changing the hash syntax just for the sake of it if that means we lose compatibility with it (I wouldn't be against not supporting it if, say, we were dealing with strings and encoding… but hash syntax surely doesn't make things harder to edit.)
Another thing is that this makes it difficult to merge the existing branches. We may as well merge fix/multiple-connection (which I never actually did because I don't know if it does fix the issue of having multiple connections, but at the very least the way it captures stdout/stderr is saner), and possibly `
Regarding the editor, looking at how Pry handles it, it's not easy to do without monkey patching. I think the cleanest way would be to first change Pry so as to allow Pry.config.editor to be a proc. Then we can redefine it to so as to call Pry::Editor.edit_tempfile_with_content on the client side.
EDIT: nevermind what I said about the editor, we can already do that. I was looking at an older version of the source code.
(Btw, you should use separate branches for separate pull requests in the future --- otherwise they will all show up together as they do here)
I separated out the sections of the module to make the gem easier to edit and update in the future.
Among the changes that aren't strictly 1-to-1:
If I can get something like this through, I intend to try and work out a way to get a remote editor to work next as that'd be insanely useful. That, and adding tests. I can do that one as well.