Feature request: Guard3 API

[e2]

I needed a single place to consider/discuss a new API for guard, which would provide more clarity, flexibility, control and robust handling of events for users, plugins and contributors.

(There's a similar request on the listen side: guard/listen#214)

Also, issues and plugins which would be affected by changes could be redirected here.

Since my knowledge is limited to Linux / inotify / vim / related listen gem internals and use cases such as: guard-cucumber, guard-rspec ...

... I'd appreciate listing issues and caveats worth considering that I don't know about.

(And bumps/+1s to know what people want/expect in how they use guard/listen).

[thibaudgg]

If we all agree that Listen will only notify content changes (so no more added/removed/modified events) I would deprecate these methods:

• Guard::Plugins#run_on_additions
• Guard::Plugins#run_on_modifications
• Guard::Plugins#run_on_removals

We could make them an alias of Guard::Plugins#run_on_changes with a deprecated message.

What do you think?

PS: See https://github.com/guard/guard/wiki/Create-a-guard guide.

[e2]

For the current API, I 100% agree.

As for a 3.0 version of the API, I haven't got to thinking about plugin callbacks.

My biggest current issue is that's it's not obvious that run_on_modifications gets the result of a watch block. (And there's the any_return parameter while allows callbacks to have objects).

I was thinking: since we no longer need to worry about event types, we could instead make sure the API gives plugins more flexibility, context and local options/parameters. (For adding capability that wouldn't require changing Guard API).

E.g.

directory('app/views') do
  watch('*.haml', foo1: 'bar1', foo2: 'bar2') { |dir, object|  "hello" }
end

could call:

run_on_modifications(block_result, watched_dir, changed_object, plugin_options)

1. block_result would be hello
2. watched_dir would be app/views (in case some plugins want to know which dir is being watched)
3. changed_object would be e.g. users/login.html.haml
4. plugin_options would be {foo1: 'bar1', foo2: 'bar2'}

I'm not even considering the naming at this point.

[e2]

I really love the "groups" functionality.

It's kind of a shame that watch itself doesn't provide so much.

Here is a list of "problems" with watch:

1. it doesn't handle globs, e.g. like Dir[]
2. it takes up lots of visual space
3. it passes matched regexp groups, so it's using the pattern for filtering files and for creating a parameter for plugin callbacks (not to intuitive and tricky)
4. it basically controls what run_* callbacks receive
5. it doesn't have access to any other useful parameters to pass to callbacks
6. any_return is an option the plugin gets (where intuitively it's something I'd say the plugin should set itself)
7. it's extremely unreadable, e.g.:

watch(%r{^features/step_definitions/(.+)_steps\.rb$}) { |m| Dir[File.join("**/#{m[1]}.feature")][0] || 'features' }

1. the whole watch block has to be repeated in every guard body in every group
2. it can't be reused since it's a method defining a watcher, and not e.g. just a pattern declaration with a block
3. it's called once per file, while the run_on* callbacks are for multiple files (counter intuitive)
4. there's no access to the plugin instance, or otherwise you could do something nicer like:

guard :rspec, foo: 'bar' do
  watch('spec/**/*_spec.rb') { |rspec, path | rspec.run(path, tag: '~slow') }
end

Personally, while realizing we're dealing with Ruby here (and with blocks), I have no idea why plugin authors should not be allowed to define their own API to use within blocks (I may be missing something).

Why not just let users run whatever API their plugin exposes?

If they break compatibility, I could then maybe fix it in my own Guardfile.

Currently, if something trivial stops working with guard-cucumber, I'd have to fork the plugin and set it up with bundler, then patch it.

(That's because in the watch block all I can do is ... compose a string and return it. )

1. It's like half the work of the plugin is done inside the watch() block - which is plugin specific, again like:

watch(%r{^features/step_definitions/(.+)_steps\.rb$}) { |m| Dir[File.join("**/#{m[1]}.feature")][0] || 'features' }

Very rarely will that be different for another project. In fact, I'm guessing the API interface between guard and guard-cucumber has been changed a lot more frequently than this string. That's not optimizing for the most common case.

Why couldn't it be something like (assuming we shouldn't assume cucumber step definitions are in ruby):

guard :cucumber do
  directory('features') do
    watch('*.rb') { |cucumber, path| cucumber.run(cucumber.guess_feature(path), tags: '~wip') }
  end
end

And if I do have some strange mapping between files and cucumber, there could be a helper method for that, specifically (too specific for a regexp to make sense) and just have cucumber with sensible default handling included that dependency:

watch('app/models/location.rb' => 'features/gps.feature')

guard :cucumber do
  watch('features/**/*.feature') # gps.feature automatically matched when location.rb changes
end

Which would require no changes to cucumber or regexps.

Based on what I browsing guard plugins, there isn't anything "brilliant" about how the plugin api is used (some implementations just run the same "refresh" or "run everything" method in every of the run* callbacks).

Ironically, the snippet templates plugins install are more complicated than the plugins ... and it should be the other way around, especially since improvements in templates never benefit existing projects.

So I'm sure there's a huge opportunity to improve the DSL, staring with getting clear about the watch method, focusing on tracking directories (instead of matching lists of changed paths with regexps).

I'm just brainstorming and still discovering lots of things about Guard that I never knew about.

I feel like the current DSL is perfectly written to solve a problem that isn't even remotely related to the common usage patterns of guard. I mean it's fantastic for configuring and managing plugin options - except it doesn't seem like Guard was meant to be a build configuration tool.

[thibaudgg]

That'll sound boring at the end, but I agree again with you.

guard :rspec, foo: 'bar' do
  watch('spec/**/*_spec.rb', foo2: 'bar2') { |rspec, path| rspec.run(path, tag: '~slow') }
end

looks good, but in that case the watch block should be used only for advanced configuration, most of the time the watch method with just Dir[] glob params should be enough. Do you agree?

What about map or match for your cucumber example?

map('app/models/location.rb' => 'features/gps.feature')

guard :cucumber do
  watch('features/**/*.feature') # gps.feature automatically matched when location.rb changes
end

If I understand well your example the watch block params change depending from where they are called: what about the dir block params in that case?

# [guard_plugin, path] when used inside a guard plugin
guard :cucumber do
  directory('features') do
    watch('*.rb') { |cucumber, path| cucumber.run(cucumber.guess_feature(path), tags: '~wip') }
  end
end

# vs

# [dir, object] when used outside a guard plugin
directory('app/views') do
  watch('*.haml', foo1: 'bar1', foo2: 'bar2') { |dir, object|  "hello" }
end

I'm not sure is good to have variable arguments for the same method.

[e2]

Very good points - thanks.

The block parameter differences are just me brainstorming on how to connect glob patterns with watch blocks with local plugin configuration overrides with plugin methods.

So I'm still confused myself.

Since I'm not sure myself how the watch() method should work, I'll stop using it in examples for a new API (if the new method would be too different, a deprecated watch() method could be added later to simulate old behavior).

I'll use listen instead of watch to avoid confusion from now on.

My mind keeps drifting back to the example in guard/listen#214:

(modified here)

directory('app', ignore: 'schema.rb') do
  directory('views') { listen(%w(*.erb *.haml *.slim), :rspec, :cucumber, :livereload) }
  directory(%w(models controllers helpers config lib) { listen('*.rb', :rspec, :rubocop, :livereload) }

  listen('config/locales/*.yml', :cucumber, :livereload)
  listen('db/**/*.rb', :migrate)
  listen('spec/**/*.rb', :rspec)

  directory('features') do
    listen('*.feature', :cucumber)
    directory(%w(support step_definitions)) { listen('*.rb', :cucumber, :rubocop) }
  end

  listen('Gemfile', :bunder)
end

Because it seems to make more sense to me that a Guardfile should contain a "directory tree layout" (instead of a grouping of plugins and options), only because it would be easier to work out what's going on and where.

(Users are probably thinking more within the context of a project subdirectory than within the context of a plugin).

Ideally, since running e.g. cucumber from the command line works by itself, adding gem "guard-cucumber" to the Gemfile should be enough to have Guard tracking the features directory.

Same with guard-rspec and guard-bundler and guard-migrate.

I also think chaining plugins and tying them to a specific watch directory is more desirable than trying to work out how to use groups to create the desired "plugin calling" order.

So with layout autodetection/conventions, from the above we'd get maybe:

directory('app', ignore: 'schema.rb') do

  # I'm not sure if this is a good example
  directory('views') do
    dep(%w(*.erb *.haml *.slim) => :livereload)
  end

  # run rubocop after these "states" (managed by plugins)
  # :coverage_ok would be a state in the coverage plugin if total coverage
  # is within tolerated limits
  dep([:cucumber_ok, :cucumber_all, :coverage_ok] => :rubocop)

  # override plugin default dependency to prevent running all after success
  nodep!(:cucumber_ok => :cucumber_all)

  # if guard-coverage would automatically set up being run after features
  # would could override that like so:
  nodep!(:cucumber_all => :coverage_all)

  # Run all features on startup
  dep(:guard_startup => :cucumber_all)

  # redefine "workflow" on our own:
  dep(:cucumber_ok => :coverage)
  dep(:coverage_ok => :rubocop)
  dep(:rubocop_ok => :coverage_all)

  # note: :coverage would "mean" :coverage_file, which means running
  # simplecov on a triggered file - as opposed to :coverage_all

  # note: plugins here have "states" and "callbacks"
  # :cucumber_ok -> means feature file worked successfully and cucumber
  # state changed to "ok"
  # :cucumber_auto -> means let cucumber decide what features to run
  # based on which files changed, so as guard-cucumber grows more "intelligent"
  # the dependencies could get more accurate, e.g. changing a step file
  # triggers calling on those features using those steps


  # Guard could run plugins in parrallel and show task summary, as they finish
  dep('config/locales/*.yml' => [:cucumber_auto, :livereload] )


  # as a last dependency, so it's added to the list
  dep('*.rb' => :rubocop)


  # NOTE: Extreme config and dependency hacking example below:

  # For seed.rb, run rubocop first, because testing this huge seed file after 
  # every change hurts productivity. and only run the related cucumber feature
  # after everything else is ok.

  # 1. Remove all dependencies related to a file - same as :ignore in
  # "directory", except it can be added with new dependencies (below)

  nodep!('db/seed.rb')

  # 2. Prevent the feature from being included in :cucumber_all
  # (:cucumber_auto would not receive the change notification from Guard)
  nodep!(:cucumber_ok => :cucumber_all, for: "seed.feature")

  # 3. Pave rubocop run on changes
  dep('db/seed.rb' => :rubocop)

  # 4. Run the given feature whenever the seed file is modified
  # `for` means skip the dependency only for this file
  # Also, note how files are relative to plugin's watch dir
  dep(:rubocop_ok => :cucumber_file, for: 'db/seed.rb') { "seed.feature" }

  # 5. Also run the feature automatically as a final test after EVERYTHING else
  dep(:coverage_all => :cucumber_file, for: "seed.feature") { "seed.feature" }

  # At this point, tests for 'db/seed.rb' are only run if the file is edited
  # or after coverage is successful for the whole project

end

This means plugins would return their "state", e.g. :cucumber_ok

Symbol -> state from plugin
String -> pattern as Dir glob
Regexp -> current guard regexp handling

So dependencies could eliminate a few configuration options.

And I think it makes sense for guard to treat FS notifications and plugin
callback results as the same thing.

Anyway, if the user can override dependencies in the Guardfile, then
guard-plugins can setup default dependencies (telling even Guard what to watch
by default).

This also means some people could ultimately prevent cucumber_all with a
dependency removal in their ~/.Guardfile, which could have an after block for
this purpose.

Anyway, I'm still trying to brainstorm an API that could be easy to implement
initially without too many future changes in plugins, while giving more control
over plugin behavior - without requiring flexiblity from the plugins themselves.

[e2]

The above would also allow plugin authors to define an API "methods" of their own (e.g. :cucumber_auto) which could help setup complex workflows without changing anything the Guard API. This allows plugins to also setup default workflows and know about "each other" (e.g. coverage + cucumber) without guard having to care.

And since guard-plugins are more related to development than deployment, a Gemfile could contain "supported" guard plugins, while the user's ~/.Guardfile could selectively "reconfigure" or "disable" them globally for every project.

Or, globally (re)define "personal dependencies" common across projects - like cucumber_all on startup, etc.

[e2]

And with dependency chains, guard could "collect" changes during the chain and provide the relevant, aggregated (and filtered) results to the next "bulk" plugin action, e.g. :cucumber_auto, which would work on a group of files.

This is basically analogous to how listen collects messages before sending them to guard, except here instead of polling for a period of time, guard would collect changes (while tasks do their thing) before firing the next ones - which would allow guard to detect loops.

[e2]

Instead of a watch block run for every modification type, in the new API, we could have the dep block called once for every plugin:

# Manual dependencies that plugin's can't work out themselves
dep('config/locales/*.yml' => 'app/model/products.rb') # so yml changes trigger everything else
dep('app/model/products.rb' => 'features/reports.feature') # for cucumber
dep('app/model/products.rb' => 'app/views/products/report.html.haml') # for livereload

# The block overrides options for just the yml file and the changes it causes
dep('config/locales/*.yml' => [:cucumber_auto, :livereload] ) do |plugin, watched_dir, array_of_objects}
  options = { notifications: :only_error} # only livereload or cucumber failures
  options.merge!{tags: ['~javascript']) if plugin.name == :cucumber
  options
end

In the above example, both cucumber and livereload would get the following list of files in array_of_objects:

config/locales/en.yml
app/model/products.rb
features/reports.feature
app/views/products/report.html.haml

Specifically, the method in the plugin could be:

def run_command(command, dir, files, options)
  # command is :cucumber_auto
  # dir is '.' (project directory, because of how the above example is set up)
  # files is ['features/reports.features', 'config/locales/en.yml', 'app/...']
  # options are merged with defaults for the duration of this call
  case command
  when :cucumber_auto
     run_with_options(options) unless previous_failures?
     ...
end

And it would be up to the plugin method which to ignore when responding (e.g. cucumber would only process features/reports.feature, while livereload would only care about report.html.haml).

Ideally, triggers would be ignored if they wouldn't be related to their watched directory, so cucumber would only get features/reports.feature here.

Note that :cucumber_auto could get called with 'features/reports.feature' with different options - depending what causes the 'cascade', e.g. directly editing features/reports.feature would call cucumber with defaults (and not the above option overrides).

So a good Guardfile would only contain "fine tuning" for file-local plugin configuration options and custom dependencies - specific to the given project.

Summary: so in terms of just the API, basically, dep blocks would do what current watch blocks do - except more explicitly and with more control. Which means we could "extend" the current API just slightly (without breaking backward compatibility) and still allow lots of flexibility down the road.

The difference is that "pattern matchers" (which is what watch is) and their blocks can be reused for multiple plugins, plugin statuses can be watched for, and dependencies can be modified allowing plugins to create default "watches".

And since notifications could be set up as dependencies:

dep(:coverage_all_ok => :notify_success) do |plugin, dir, files|
{ message: Guard.plugin(:coverage).last_status , icon: "exclamation" }
end

(Note: "files" parameter could be nil, if coverage was not called by a file change, but e.g. directly).

or replaced:

# Let's say non-error cucumber notifications distract while using livereload
dep!(:cucumber_ok => :notify_success) do |plugin, dir, files|
  logger.info Guard.plugin(:cucumber).last_status 
  { message: nil } #show no notification
end

Then notifications could be treated like any other plugin, and extracted into one.

[e2]

TL;DR: Crap, I'm considering a big rewrite of Guard - debugging and keeping tests working takes too much time.

Everything is too tightly coupled to major refactoring (necessary for new features) to be feasible.

For example, there shouldn't be any non-trivial singleton objects (e.g. the setuper module), because even slight changes cause a chain reaction of failing unit tests impossible to fix without major debugging and rewriting - because tests aren't isolated (due to singleton).

Example: tests mostly run ok, unless e.g. the signal handling tests run before the interactor enabled/disabled tests ... because the signal handling call Setuper.interator (in Commander), which sets up the interactor, which causes the interactor tests to fail - because the the @interactor is already set, so it isn't created, so the test for Interactor.new fails.

Then, much of the refactoring relies on changing the setuper ... where even the slightest change brings down half of the test suites.

I thought about adding cucumber tests based on aruba - to keep the functionality working + keeping backward compatibility and using unit tests only for edge cases.

Another option would be to "rewrite from scratch" - meaning starting a new branch, removing all files and slowly adding unit tests as classes, and refactoring them before adding new ones - that way it would be easier to decouple classes without hours and hours of debugging broken unrelated unit tests.

Currently, the most mind-bending functionality (to implement correctly) is (re)configuration management + reloading and the related unit tests.

Also, I think the class/module documentation can be more or less dropped (everything that's obvious), because it makes the code harder to navigate (6 lines of code for a one-line attr_reader/writer who's name says what it does), I doubt people actually read the docs (as opposed to reading the code), and the docs are hard to navigate anyway (in a way to "understand" how things work together and where to look for what).

I'm a little "torn" between touching as little as possible (to keep a working master at all times) and rewriting basically every single class and it's test - since I'm having a hard time working out a middle ground.

I'm thinking that since the current version is stable enough, issues are few and non-severe (so we can always backport fixes/improvements and release from a stable branch), a major rewrite is the best option - because there's years of thinking, ideas and "experience" already in the code suggesting a better architecture, while there's been so many "tweaks" to maintain backward compatibility and so many minor features I doubt are useful to keep (e.g. programatically reloading a Guardfile).

Instead, a rewrite would allow taking all the knowledge that's there, reducing/simplifying the codebase, decoupling, changing the architecture (e.g. making the interactor a "long-running task" just like any other task - and allowing tasks to be interrupted just like the interactor, using a micro-"reactor" for asynchronously handling events and managing tasks), improving the tests, and thus making contribution easier (the biggest problem now).

E.g. one place where I'm still lost is scopes/groups functionality - where there are a few outstanding bugs worth fixing (e.g. merging duplicate groups, so you can combine rspec options in ~/.guard.rb and Guardfile).

I think backward compatibility is doable after the refactoring (e.g. by conditionally including a "Compatibility" module where needed) - so that would be less of an issue than it is now.

The downside is: massive changes, unreadable diffs, rebasing not possible - almost an unrelated project considering the difference in commit history.

E.g. ::Guard.start(options) would become ::Guard.new(options).start, ::Guard.setup(options) would become: ::Guard.new(options) and freezing/protecting options. This alone would break most (if not all) of the tests, thousands of lines of code would have to be changed - but when done properly - tests would be isolated and deterministic, setup could be simplified, classes decoupled, tests reduced, options kept in one place, etc.

Of course, such a change would be hard to break into "understandable" commits, and reviewing diffs would make almost no sense - as opposed to reviewing refactored and simplified classes after the changes.

Then again - since the project is mature and there's no ongoing active development - this may be the perfect time for leveraging all the knowledge, experience and "learning" that's reflected in the code to make the most of all the effort that was put in.

Any thoughts or comments on this would be much appreciated.

[rymai]

I'm in favour of starting Guard 3 "from scratch", in a new branch. I've never liked the singleton stuff, it has always made the tests very brittle etc. That's actually what I've started to do with Guard 2 (i.e. extracting logic in separate classes).

For me it's a GO (don't hesitate to open a pull-request early so we can see the progression and give the feedback directly).

Regarding the inline documentation, I tend to agree with you, a vast majority of the documented stuff shouldn't documentation at all.

[e2]

Thanks @rymai!

Yeah, I was thinking about having a Guard::Session to encapsulate the settings and try and make the interactor, runner and notifiers "plugins", managed by a reactor (in progress). After that, things could be decoupled and reorganized without breaking everything.

Here's a "case study" for refactoring (evaluator): https://github.com/guard/guard/tree/refactoring_attempt
And here's the file after refactoring: https://github.com/guard/guard/blob/035ffccc16ca1bd811d02ed7bfc8b0fe4340a246/lib/guard/guardfile/evaluator.rb (the diffs don't show anything useful).

I'm most happy with the changes here: https://github.com/guard/guard/compare/synchronous_guard
(Still needs a lot of refactoring - and the reactor will make a lot of changes unecessary)
Here's what the interactor looks like "now": https://github.com/guard/guard/blob/cc79f6de3c1c5952c470cc98729da50b6c673c99/lib/guard/interactor.rb (I'd rename the "foreground" and "background" methods to "show" and "hide" or back to "start" and "stop".)

In fact, I'm using the synchronous_guard branch for all my current projects - both to find possible bugs I introduced, and as a base for improvements. This is the branch I was planning to use for guard 3 and merging to master (while it's 100% backward compatible) - that's because I'd prefer to patch from this to make it ready for a release (instead of creating a stable branch separate from master and maintaining both).

So my plan is:

1. keep master available for release, but merge and use sychronous_guard once it's deemed "production ready".
2. start a new branch, do a git mv of all files (to preserve history somewhat), create cucumber+aruba tests for basic Guard functionality (bare minimum possible: basic evaluation + watch + do something) - which will require a lot of code, anyway
3. refactor that minimum to leave room for 3.x Guard API somehow (and "backward compatibility" for Listen 2.x, since Listen 3.x is still WIP)
4. restore functionality by priority (interactor, commands, scope, groups, etc.)

Meanwhile, I'll also get the ruby warnings and rubocop offenses as low as possible for the whole codebase - while some are just a nuisance, overall they help improve the code and catch a lot of mistakes and design problems. There may be other code related features worth looking into, e.g. more strict rspec options, etc.

As for documentation, I'll be cutting most of it out and instead - inserting comments I wish I had myself (saying why a method is necessary, where it's called from and why, why X is important to stay as it is, what can be refactored safely, what should be removed in the future, etc.).

[thibaudgg]

@e2 that sounds like a great plan. Go for a big rewrite and less docs!