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

Seperate module #54

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

Conversation

baweaver
Copy link

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:

  • Converted Client to a class
  • Changed to JSON hash Syntax (Ruby 1.9+)
  • Changed english operators to logical (Control flow issues)
  • Updated setup and teardown to only change stout and stderr if needed.

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.

@Mon-Ouie
Copy link
Owner

Mon-Ouie commented Jun 7, 2014

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)

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.

2 participants