Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Listen gem integration #245

Closed
thibaudgg opened this Issue · 29 comments

5 participants

@thibaudgg
Owner

Now that Listen seems ready to be used, it's time to integrate it inside Guard, yeah!

Doing so will engender some important change in Guard and I want to get your feedback about them. Listen gem will give us, modified, added and removed file paths, right now Guard only has modified and added files (given together, via run_on_change) and removed/moved file paths (via run_on_deletion) with the -A/--watch-all-modifications option (but there's an issue #212 with the implementation).

I propose:

  • -A/--watch-all-modifications option should be removed as Listen gem will always watch all changes.
  • run_on_change & run_on_deletion should be deprecated and replaced by run_on_addition, run_on_modification and run_on_removals (more about that below).
  • -I/--no-vendor option could be removed as Listen gem handle that.
  • ext & lib/vendor path could be removed, obviously.

I'm aware that deprecating run_on_change & run_on_deletion will warns on all guards but it will really be easy to upgrade (one or two methods to rename) for everyone and I think the whole guards ecosystem will benefice of that. We could just show a deprecation warning message for each deprecated guards when launching Guard with something like:

DEPRECATION WARNING [Guard::XXX]: run_on_change is deprecated and will be removed in future releases, is it really easy to upgrade, please read "wiki-page-link"

What do you think of that, do you see other alternative?

@thibaudgg thibaudgg was assigned
@netzpirat
Owner

I 100% agree to everything proposed. This is a chance to have the most used Guards upgraded to the latest Guard API. We could write a migration guide and put it to the wiki and add also information how errors should be handler (with throw) and give examples for most common patterns.

@Maher4Ever

Although this might be a big change to a core building-block of Guard, most users won't notice it if the maintainers of the most used Guard release new versions after this change. I noticed that the names of the new methods use the singular form in run_on_addition, run_on_modification and the plural form in run_on_removals. I think that using the plural form in all the methods is better as these methods get called with an array of files.

Beside from that small observation, all your proposals are great. I'm looking forward to using the new Guard with Listen and see how they play together in the long run :)

@thibaudgg
Owner

Yeah +1 for the plural form:

  • run_on_additions
  • run_on_modifications
  • run_on_removals

I'll attack that on the listen branch when I found some time :)

@rymai
Owner

Awesome guys!

@thibaudgg
Owner

Ok, I begun the Listen gem integration on the listen branch. It's already working but only for file modification, I'll continue when I'll have some free time (and not super tired!), so feel free to commits on that branch :)

One point through. What do you think about adding a global run_on_changes (note the s on change) method to Guard, to remove duplication on guards implementation. Something like:

module Guard
  class Guard

    def run_on_changes(paths)
      # general behavior
    end

    def run_on_additions(paths)
      run_on_changes(path)
    end
    def run_on_modifications(paths)
      run_on_changes(path)
    end
    def run_on_removals(paths)
      run_on_changes(paths)
    end

  end
end

So a guard could overwrite just that method and having the same behavior across all changes. Great no?

@Maher4Ever

+1 to adding run_on_changes. From your example it seems that the developer should manually call it in the code, but wouldn't it be nicer if we called it automatically? After all, the code in that method should always be shared.

@thibaudgg
Owner

Yeah that's already the case, because guard will inherits from this implementation.

@Maher4Ever

If I see it correctly, that would require guards to call super in all the three method, right? I was thinking that we could always call run_on_changes from this method to avoid the super call. What do you think about it?

@Maher4Ever

BTW: I'm working on the specs for the runner, but I was wondering about the lock used here, what is it for? I can't see any resource being shared between the interactors and the runner.

@rymai
Owner

I think the example above is not relevant since when a guard want to have the same behavior regardless of the change type (addition, modification, removel), it'll just have to override #run_on_changes (and not the others).
The example @thibaudgg gave is actually almost the default implementation if you don't override any methods.

@thibaudgg
Owner

+1 for @rymai explanation.
@Maher4Ever the lock is to block new run_on_changes call (if I remember well).

PS: Thanks to work on runner specs!

@Maher4Ever

@rymai Thanks for pointing that out :). I indeed based my question on the example above which lead to me thinking about guard's implementations.

@thibaudgg I see now why the lock is used. It's being used more like a semaphore than a mutex, but that's beside the point. Thanks for clearing that up.

@Maher4Ever

The listen gem integration process seems to approach its end. Tests pass on (almost) all supported Ruby versions for the runner. Rubinus has a problem with the eql operator of Rspec v2.9. This commit updates Rspec for Guard and starting with it some of the tests fail. The problem is a bit tricky to reproduce, but It seems that Rubinus's compiler can't find RSpec::Matchers::BuiltIn::Eql sometimes. You will see the specs fail if you are lucky the first time, or you could try the following command: rm -rf ~/.rbx && bundle exec rspec.

I couldn't pinpoint the source of the problem (to be able to report it), but for our specs we can just replace all eql calls with == in the specs. @thibaudgg @netzpirat @rymai is that OK with you?

@rymai
Owner

NiceW

I suggest to simply replace eql by eq! :)

@rymai
Owner

Ok I've pushed a few improvements, in particular I've move all the Guardfile-template-generation methods into a new Guard::Guardfile class. :)

Also, we'll have to implement Guard::Runner.deprecation_warning! ;-)

@thibaudgg
Owner

Nice progression! We still need to work on deprecation warnings and the wiki page with info about how to upgrade existing guards.

@Maher4Ever

I just pushed an implementation of the deprecation_warning for the runner. The message could be improved be adding a link to the wiki-page of the upgrade instructions.

@rymai I already tried replacing eql with eq, but rspec seems to also have a problem with it on rubinius. The only method that works for now is ==.

@rymai
Owner

Awesome deprecation notices! :)

Ok for the eql/eq shit, let's use == instead then? Or is this something that'll be fixed in RSpec at some point?

@Maher4Ever

Great. I'm sure it won't take a lot of time before rspec fixes the eql problem. So in the meantime we'll just use ==.

Update: Now we are green all the way :) http://travis-ci.org/#!/guard/guard/builds/1113274

@thibaudgg
Owner

@Maher4Ever nice work! What do you thinks about adding a link to a wiki page with more explanation about how to upgrade (with code examples) in the deprecation warnings? I think we really need to help guards creators to upgrade to Guard 1.1 quickly.

@Maher4Ever

Thank you. About the wiki-page: sure thing, helping guards developers is very important considering the amount of guards out there. I'll see what I can come up with :)

@Maher4Ever

I've written a small upgrade guide to Guard v1.1. It is an initial draft, so feel free to change it as you see fit :)

@rymai
Owner

Wow that's great thank you! :)

@ttilley

is it just me or is @Maher4Ever kicking some ass lately? ^_^b

@netzpirat
Owner

Thanks @Maher4Ever for this great guide!

@thibaudgg
Owner

Yeah it's really great! @netzpirat do you think we should also add some note about properly throw errors when something goes wrong on run_on_*** methods?

@thibaudgg
Owner

Ah, we also need to deprecate

  • --no-vendor
  • --watch-all-modifications

command line options (and explain that now --watch-all-modifications is always active)

@Maher4Ever

Glad you all liked the guide :). I'll add a link to it in the deprecation warning.

Update: I'll wait until the decisions is made about the other things that should be deprecated.

@netzpirat
Owner

The upgrade guide includes now a section about proper error handling. Kudos to @thibaudgg and @Maher4Ever for Listen and also the integration into Guard.

@netzpirat netzpirat closed this
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.