Guard dependencies #97

Closed
jamesarosen opened this Issue Jun 27, 2011 · 30 comments

Projects

None yet

6 participants

@jamesarosen

It would be nice to be able to have earlier guards prevent later ones from firing for a given file change. For example, if a Javascript file doesn't pass JSHint, I don't want my asset-compiler to run.

@thibaudgg
Guard member

It should be easy to implement, Guard::Guard#run_on_change(paths) method return a boolean so if it's false we can break the loop in Guard.run_on_change_for_all_guards(files). Please can you try to implement it and make a pull request. Thanks

@yannlugrin
Guard member

This change can break actuals guard, no? Implement it before a version 1.0 is not a good idea if is true.

If you test if return value is false, and not just evaluate has false, result is probably less serious, but I'm not sure is a good idea anyway.

@netzpirat

I'm with @yannlugrin.

This is a dangerous request, since it may break the whole Guard ecosystem. Sure, the example in the README returned always true from the various guards, but it was never stated that these methods should return a boolean indicating the status of the action and more importantly the return value was never evaluated.

I took three Guards (guard-minitest, guard-rspec and guard-coffeescript) and looked at the main Guard methods if they return a boolean indication the action success. As result, only guard-minitest is done right, the other two guards return a wrong value which would lead to unexpected results.

So be prepared to go through all available guards and fix them if necessary, or the joy of using Guard will be lowered.

@thibaudgg
Guard member

Ok @yannlugrin and @netzpirat made a good point. I think we can keep that for version 1.0. Seems ok for you @jamesarosen?

@netzpirat

I find this issue very interesting, since it would enable us to do stuff we discussed in #14. My concern was only to not underestimate the impact it has. From the semantic version philosophy it should be done before 1.0.0, but it should be carefully planned:

  1. Enhance documentation and make it clear what each guard method MUST return.
  2. Go over all available Guards and check if it conforms to the new rules.
  3. Implement the change within Guard
@jamesarosen

I totally agree with nearly everything said above, most notably:

  • the most obvious and elegant way to implement it would be to have returning false from any of the calls stop the chain for that change
  • using the return value will cause breakage since it wasn't the documented behavior

Waiting until v1.0 to institute the change is a perfectly reasonable solution, particularly if that's not expected to be that far off. There are a couple of other variations that would work as well but aren't as elegant:

  • have guards declare whether returning false is meant to be a cancellation, perhaps by include Guard::Cancelling or setting a false_cancels_chain class attribute to true. The default would be that this behavior is not intended (at least until v1.0).
  • add some sort of change object as an optional additional parameter to #start, #reload, etc. that has a #cancel method that Guard#start and #run_on_change_for_all_guards check between guards.

Again, neither of the alternatives I suggest are as nice, but they are a little safer. Either could be implemented in a minor release without breaking any existing guards.

@thibaudgg
Guard member

Ok, I thin we can do that for v0.6 with a beta/rc period (v0.5 is just around the corner). With:

  • A deprecation warning when a method isn't returning a boolean, and only break the loop when a "true" false returned.
  • An update of the documentation
  • A mail/issue to all guards that aren't compliant with the new rules

Sounds good to you?

@netzpirat

This would be a perfect use case for the Google group. Is there a way you can send a message to all members of the Guard organization? We should bring all Guard developers onto the Google group for exactly such cases, we're only 6 members for now :(

@thibaudgg
Guard member

I'm afraid there is not at this time. We should recommend to subscribe to the google group when we sent the mail to all guard's members (one by one)

@jamesarosen

Speaking of groups, it might be worth considering only letting a Guard cancel a change within a group. Or that might be too complex. I'm not sure whether it muddies the meaning of "groups" or enhances it...

@johnbintz
Guard member

I think I'd want it to cancel within a group, but I need to think about it more. One use case I'd use: if I have a group chained like this:

  • coffeescript
  • compass
  • livereload

I wouldn't want to trigger the LiveReload if either of the prior Guards fail. Same if I chain this way:

  • coffeescript
  • jasmine-headless-webkit with jasmine.yml configured to use the compiled JS files rather than raw CoffeeScript for the app.

If set up correctly, it would prevent some redundant things from happening (browser reloading a broken Compass stylesheet or JHW and CoffeeScript complaining about the same syntax error in the same file).

I'm sure there are disadvantages with this arrangement, though. I just haven't pondered enough to know them. :)

@netzpirat

I agree that the dependency should be handled only within a group, although it makes the change more complex, since Guard doesn't know anything about groups at runtime. Disabled groups are just not evaluated when the DSL is evaluated.

@rymai
Guard member

Hi,

yeah we would have to find a way to store each guard's group and keep the same order of groups and guards as declared in the Guardfile. Also, any structure modification to @guards = [] could be harmful for some existing guards that access the guards list.

Maybe we could pass a group to Guard::Guard#initialize (in the options hash) and store groups in @groups = []. Then call (at the current line 46):

groups.each do |group| # we should set group to "default" (for instance) if no group present
  guards.select { |guard| guard.options[:group] == group }.each do |guard| # simplified since it should handle cases when guards have no group...
    supervised_task(guard, :run_on_change, paths) }
    # with new logic when supervised_task return false here
  end
end

Any thoughts?

@netzpirat

Very good point about being careful with structure modification of @guard. Your proposed solution is thus elegant and has the advantage that we don't have to deal with inconsistency of Hash ordering in different Ruby versions, since Array have always been ordered. Having the groups available would also enable us to implement issue #52 more easily.

@rymai rymai was assigned Jul 21, 2011
@rymai
Guard member

Hi guys, I started to hack around for this feature (I've already added the @groups attribute here). I think we need a new option for the Dsl.group method to enable dependency between guards inside a group since sometimes groups are not necessarily composed of dependent guards.

Such option could be:

group :backend, :halt_on_fail => true do
  guard :spork
  guard :rspec
end

# or

group :backend, :chained => true do
  guard :spork
  guard :rspec
end

Note: With such a name, this option should default to false.

Please give ideas for an appropriate name for this option!

PS: Do you think I should mail the Google group (14 people now) for this? :)

@thibaudgg
Guard member

+1 for Google Group mailing

@johnbintz
Guard member

+1 for halt_on_fail, +1 for the Google mailing as well. Will this still be managed by returning false from the run_* methods for failure and something not === false for success?

@jamesarosen

One option to make for better backwards-compatibility would be to use a raise or even a throw instead of returning false to indicate a stop. (I personally think this is a perfect place to use throw, which doesn't see a lot of action in Ruby.)

@rymai rymai pushed a commit that referenced this issue Sep 15, 2011
Rémy Coutable First implementation of #97 "Guard dependencies". b1b6992
@rymai
Guard member

Hey guys!

I've just pushed a first implementation of this feature in the new guard_dependencies branch.

I think, a refactoring of groups into a Guard::Group class will be needed next.

Let me know what you think (I've used your suggestion @jamesarosen for the throw, very cool idea thanks!).

@netzpirat

Very well done Rémy, thanks a lot. This serves also as a good basis for other ideas like #52. So then I need to upgrade my Guards to throw :task_has_failed :-)

Next we need to document this for Guard developers and I'd like to take the opportunity to add full YARD support when @thibaudgg and @rymai agrees.

@rymai
Guard member
@netzpirat

It doesn't matter whether your Yard branch is up to date or not. I anyway would go through Guard class by class, method by method, and see what is in your branch, transfer and update it where necessary.

@thibaudgg
Guard member

Ok great, +1 for YARD and documentation for Guard developers :)

@rymai
Guard member

Ok, I've pushed it here, in reality I almost only documented the Guard::Dsl class. So feel free to document Guard! :)

@netzpirat

Thanks. I've already annotated 100% with yardoc and pushed it to master. I've still some limited knowledge in some areas of Guard, so I'd be happy, whenever you touch a method, please check the docs and add the not-so-obvious-things you've got from your experience with Guard. This then makes the docs really useful.

There are still some minor formatting issues that I've to address, but at least it's a good starting point for the future: http://rdoc.info/github/guard/guard/master/frames There is also a problem with the GitHub flavored markdown that is not supported on rubydoc.info. I've to do more investigation on that, but in the worst case we should drop the fancy colorization in the readme to make it work.

@thibaudgg
Guard member

Wooo, that's just awesome, thanks @netzpirat!

@rymai
Guard member

This is simply plain awesomeness! Thank you Michael!

@rymai
Guard member

I've now merged the branch into master. Next step is real testing and to add throw :task_has_failed into existing Guards.

@jamesarosen

Love it!

@thibaudgg
Guard member

Ok Guard 0.8.0 is out with this new feature, now we'll just need to add some documentation (README) about it :)

@thibaudgg thibaudgg closed this Sep 28, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment