Skip to content

Use Pry as Guard interactor #327

Merged
merged 25 commits into from Oct 22, 2012

8 participants

@netzpirat

This is the initial integration of Pry as replacement of the Guard interactor (:simple, :readline and :coolline are all gone). Pry itself makes use of rb-readline and coolline whenever they are available, so support for these gems is still available.

Guard provides its own set of Pry commands that can be used to control Guard in the same way as before. However Enter doesn't trigger the run_all action anymore, instead use the all command.

The biggest advantage of using Pry is that it comes with a powerful plugin system that allows you to customize and extend the console. This will allow Guard users to release gems that provide extended functionality to Guard, as it's needed by for example by #321 and #305.

Pry will evaluate ~/.guardrc in addition to the standard ~/.pryrc and ./.pryrc before the session starts, which allows the customization of prompts, commands, aliases, etc.

I didn't provide the short hand commands c, s, p, etc. by intention to avoid variable and method collisions (remember, we have now a full featured Ruby REPL with shell access), because it's so easy to customize it. Just add

Pry.config.commands.alias_command 'n', 'notification'

to ~/.guardrc and you're fine.

There's still some work to do like missing commands specs and a better Pry session restore, but it works and I'd like to have people play with it and give feedback.

I recommend to have a look at the Pry Wiki to see what's possible.

@netzpirat netzpirat was assigned Sep 17, 2012
netzpirat and others added some commits Sep 17, 2012
@netzpirat netzpirat Fix wrong param name. 06a1e19
@rking rking Hook instead of RC_FILES; Increased verbosity.
Pry::RC_FILES went away recently, in this commit:
  pry/pry@d830ebb

The hooks should work both in 0.9.10 and HEAD pry.

Also, gave the user a hint about ~/.guardrc, because it changes the
interactor experience quite a bit with/without one.
2837fbd
@thibaudgg
Guard member

It seems to be pure awesomeness, I'll give it a try this week. Thanks!

@martco
martco commented Sep 21, 2012

👍

@netzpirat

So all specs are now in place and the pull request should be fine to merge. However I'd like to have some more feedback how it works for you guys.

For me it works mostly, but I have seldom hangs of the interactor, so it won't read any more input. Do other people also see this behavior? I have spent some hours but didn't discover why this happens sometimes...

@thibaudgg
Guard member

Yeah @rymai and I also get (quite frequently sadly) hangs of the interactor (no more key input accepted).
I'm afraid that a showstopper until it's fixed no?

@netzpirat

Yes, we can't merge until the nasty bug is resolved. I've already spent some hours to find the cause for that bug, but without success. I'll keep the branch in sync, until I (or someone else) has time to dig into it an fix it. I'm on holiday for the next two weeks, so I won't be able to put any work into it before the 15th of October.

@thibaudgg
Guard member

@netzpirat great, enjoy your holiday!

@rking
rking commented on 707a1b0 Sep 27, 2012

The reason I made that output show interactively is because I think the users that are used to the short commands are going to get this version and be puzzled about what happened.

Yes, I understand that reason and I think we're not at the end of the discussion how to solve it in the best way for Guard users. What I can say from my time as Guard maintainer is, that many people don't like verbose messages that acts as guardian for something. We've discussed many Guard messages (bundler, deprecations, etc.) over and over on the issue tracker.

I'm thinking of making the shorthand commands enabled by default and either provide information in the README on how to unalias the shorthand commands or even better, provide a command to enable/disable the aliases. This would give "normal" users the known behavior of the interactor, but would allow "power" users to switch them off for a debug session. Another solution that comes to mind would be that guard init installs a default ~/.guardrc, but this would not catch existing users with existing projects.

I'd like to hear your thought on this.

Upgrade Experience

I think the basic user impression of the new interactor is merely that it has a funky prompt. Everything else should be smooth and familiar.

So, yes, I advocate having the default commands be there.

Commands vs. Code Conflict

In general, with Pry, you have this competition between wanting short commands and wanting to be able to express Ruby. If a command parse succeeds on a line, that line is considered a command, not a line to eval. There are a few things that mitigate this problem for the Guard interactor case:

  1. The interactor pry prompt doesn't load your system's code, so it's not like it doubles as a rails c. Therefore, though it's handy to sketch out bits of Ruby, it's not trying to be the projects main REPL.

  2. You can still get the Pry parser to give you your eval by making slightly more complex input. So if there's an r command and variable both, you can still see the variable with something like ;r. This is maybe a bit unintuitive, but I think most users can figure it out (or do x = r and examine x instead).

Unaliasing/uncommanding is definitely possible

E.g. look at the ,, toggle command

For this case, though, I don't know if people will want that.

It would probably be best to just have a "don't add the commands" line for
~/.guardrc's.

By the way

Have you used pry-rescue like this (or plymouth) to automatically REPL upon test failures?

It's superawesome. IMO it's the way of the future. But there are some guard-related issues that need to be hammered out, I guess on a separate Issue.

The foremost of which is, when you have a pry prompt sitting there on a failing test, then you edit the files, at that point Guard is inactive, so you have to manually switch back to that pry and manually ^d it, then after that maybe Guard does the right thing. It's actually a pretty busted process at this point.

But it's a related topic, so I'm starting the question here.

I think the basic user impression of the new interactor is merely that it has a funky prompt.

That made me smile, it's so true.

Everything else should be smooth and familiar. ... it's not trying to be the projects main REPL

Again, you hit the nail on its head. Guard adds now the aliases by default and creates commands to run all of a single group or a plugin. So the user experience is the same as before.

I really like to play with pry-rescue, but I currently work mostly on the frontend with CoffeeScript and guard-jasmine. But time will change that again... But anyway, start/stop Pry in a thread is currently very unreliable and needs some more work to make it a smooth experience.

@cs3b
cs3b commented Sep 27, 2012

+1

@rking
rking commented Oct 18, 2012

Are you 100% sure you get hangs?

What I see from time to time is echo being off. When I can't see what I'm doing I just run this:

`stty echo`

Then it fixes it. At one point I poked around guard's stty stuff… not sure what any of it is really doing.

@netzpirat

OMG, you're a genius @rking You're right! It doesn't hang, just no echo. I restored the old terminal helper code and I haven't seen the problem until then. @guard/core-team Can you give it a try?

@rymai
Guard member
rymai commented Oct 18, 2012

It seems to work, but I'll use it tomorrow all day to be sure! :)

Thanks!

@netzpirat

That would be great @rymai. Since I'm back from holiday, my work is just making phone calls, visiting customers and write emails 😢 So sadly no Guard running all day long.

netzpirat added some commits Oct 19, 2012
@netzpirat

@rymai Did you had the chance to run the pry interactor for some time last week?

@thibaudgg
Guard member

@netzpirat for me it seems to works perfectly.

@rymai
Guard member
rymai commented Oct 22, 2012

@netzpirat Yep, seems to work fine!

@netzpirat

Great! I will merge today all pull request you're fine with.

@thibaudgg
Guard member

Perfect, 1.5.0 for today!

@netzpirat netzpirat merged commit 4a9f16a into master Oct 22, 2012

1 check passed

Details default The Travis build passed
@ches
ches commented Nov 15, 2012

This is awesome guys, thanks for the work.

One thing though regarding the upgrade experience: Guard now spews deprecation warnings about usage of interactor, notification, etc. for configuration in the Guardfile, pointing users toward ~/.guardrc. Yet the DSL methods are still the primary thing documented in the README, and there's really nothing I can find suggesting how to configure things in ~/.guardrc, examples, etc. The DSL methods don't work there. The DSL methods do still work to do user-local configuration in ~/.guard.rb (though of course you still see deprecation warnings). Moving this stuff to user preferences where it can be out of a project's version control is great, but the state of things right now is kind of circular and confusing, and I'm sure ~/.guard.rb is not really intended to be the place for configuration.

There is a pointer to the Pry docs for things one might do with ~/.pryrc as a hint for ~/.guardrc, but as discussed in the thread here, it shouldn't be Guard's interest to replace a project's REPL, so it seems like for most Guard users extending the shell isn't going to be a very common need compared to simple configuration.

So, I think the problem here is primarily just one of updating the blessed documentation 😉 Or maybe also the DSL should be loaded when evaluating ~/.guardrc? If I'm confused about the intent, please point me toward the light 😄

@netzpirat

@ches As developer we never really have an upgrade, since we continuously move forward, so thanks for your feedback.

There is no deprecation message for the notification DSL method, so I'm wondering what you mean with this. The main change from 1.4 to 1.5 was the removal of three different interactors that have been replaced by a single Pry interactor, thus the deprecation message. I think it's great to have less choice.

I just re-read the deprecation warning and simplified it, because it was not quite correct and the hint for the Pry customization is just not necessary, since it targets more advanced use of Guard. In fact this was also one of the drivers for the change: people wanted to have more control over the workflow with Guard, and a programmable interactor using the Guard API makes a lot of customization possible. It should not replace your main REPL, but it can be very useful for improving your test workflow for example.

We have a short sentence in the README about the ~/.guardrc:

Further Guard specific customizations can be made in ~/.guardrc that will be evaluated prior the Pry session is started (~/.pryrc is ignored). This allows you to make use of the Pry plugin architecture to provide custom commands and extend Guard for your own needs and distribute as a gem. Please have a look at the Pry Wiki for more information.

If it's not clear to you that the ~/.guardrc is a Guard specific ~/.pryrc to customize Pry and you can write Pry plugins, then please help us to write this in a more clear way (Guard core speaks French, German and Dutch). Pull requests for better wording, fix bad grammer, etc. is very welcome, since we can't do any better.

The quote from the README has also a link to the Pry wiki and if you follow it, then I see pages about

and they contain tons of information and examples, and there is even a lot more available. What are you missing?

@ches
ches commented Nov 18, 2012

@netzpirat Thanks for the detailed reply.

There is no deprecation message for the notification DSL method

You're right, apologies, it was only interactor. Totally agree that streamlining the choices is a good thing, plus it just works more reliably I think.

Regarding your updated deprecation message, in my case I'm only wanting to set a history file location:

interactor :history_file => 'my preferred location'

It sounds like I shouldn't be getting a deprecation warning for this usage at all? Or what would be the recommended way/place to configure this now? I think that's my main question and what I found confusing/circular about the docs and warning.

I noted the mention of the Pry customization in the README. Personally, I'm a Pry user and I'm familiar with customizing and extending it, so to me at least, the wording and the expressed intent is fairly clear. I think what tripped me up about it is the ordering of the information: that mention of ~/.pryrc now precedes other info on configuring Guard in the README, so it seems to imply a precedence of what is now preferred. As you say, this is probably only for more advanced use really, and something like configuring Guard notifications is orthogonal to customizing Guard's Pry instances, I think (you might wish to turn the interactor off entirely, and evaluating ~/.pryrc then wouldn't make sense). So basically, it's still not clear to me where one should set global configuration like my history file example or other similar things where options shouldn't be forced on everyone on a project through the Guardfile kept in version control. Once I understand that, I could try to work up some doc tweaks to discuss on a pull request.

@ches
ches commented Nov 18, 2012

So in conclusion, maybe ~/.guard.rb really is the right place for what I'm asking, but I think it's hard to arrive at that from the current docs, and the deprecation warning gave a further feeling that it wasn't right.

@netzpirat

The interactor deprecation warning is only shown when you pass invalid arguments, like the previous :readline and :coolline. You can either place it on a per project basis in your Guardfile or for all projects in ~/.guard.rb.

@rking rking referenced this pull request Dec 26, 2012
Closed

guard-unpry #344

@coveralls

Coverage Status

Changes Unknown when pulling 4a9f16a on interactor/pry into ** on master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.