rubocop / guard-rubocop

Guard plugin for RuboCop
MIT License
262 stars 55 forks source link

Use Guard::Compat + update to RSpec 3.1 #19

Closed e2 closed 7 years ago

e2 commented 9 years ago

Also:

yujinakayama commented 9 years ago

What's the rationale for this? Guard::Compat is described as "test helper" but it's used as a runtime dependency. Also I don't quite understand the need for Compat::UI.

e2 commented 9 years ago

It's necessary as runtime because it protects plugins from internal implementation changes in Guard - and it does that by calling the right methods in Guard. This prevents "everything" in Guard from being "part of the API".

Also, this improves testing, because tests do not need to initialize Guard to work (proper stubbing, transparent to plugin developers).

Guard is not a "library" for plugins - it's the other way around: plugins are "add on libraries" for Guard. So it's not plugins that should care about Guard API, plugins should implement the API Guard expects.

Guard Compat::UI works around very difficult problem requiring files - because deprecated methods have different kinds of "require" statements and dependencies inside Guard.

Guard::Compat::UI protects plugins from breaking when Guard's "require" statements will change.

In short Guard::Compat works around the design flaws in Guard - plugin inheritance, interdependent classes, global state, etc. Guard::Compat can also make plugin future-compatible (so any situations like this can be avoided simply by adapting the implementation inside Guard::Compat).

ic commented 8 years ago

This is an old story, but it affects running the test suite with Guard 2.13 (with 2.1, it still works). I have not dug out the exact tipping point in Guard's versions, but it seems guard-compat is the Guard community way going forward.

I have done the port to guard-compat before finding this PR, and I can confirm the plugin and the test suite work fine with it.

At the very least the test suite should still run, so PR #24 is a simple proposal: Guard 2.13 differs from 2.1 on the initialization code. The Guard class must be setup to prepare some internal variables (here we talk about @state). So PR #24 is just about setting up Guard in the spec_helper.rb file, and this allows the tests to run again.

e2 commented 8 years ago

@yujinakayama - is there anything else I can change to get this merged and released?

guard-compat was created so Guard's API can be changed and improved without having to modify all the plugins out there (again).

Basically, any Guard plugin not using guard-compat may not work for Guard 3.x, but every plugin which uses guard-compat should work without any changes.

One big change is extracting Guard::UI into a new project. Just like the notifier was extracted into the Notiffany project. This is why it's important to use Compat::UI instead of Guard::UI.

Guard was extremely "stateful" up to now - and it took a tremendous amount of time to try to fix this without breaking all the plugins.

@ic - the proper way to unit test Guard plugins is to not use Guard at all. If tests require Guard.setup, then there's simply not enough mocking/stubbing.

Check out other projects like guard-rspec, which properly and completely stub out Guard. This is important, because it allows using guard-rspec to test guard-rspec, or guard-minitest to test guard-minitest.

Inheritance for Guard plugins wasn't a good design decision, even though it seem a "natural fit".

The only thing a Guard plugin should need from guard is the Guard module name - and this is provided by Guard::Compat anyway.

I also plan to remove Guard::Plugin from Guard itself, completely. This is because "plugins use guard" and not the other way around. Also, there's no reason Guard plugins can't be used standalone. (Without Guard).

ic commented 8 years ago

Thank you for the detailed explanation. It makes sense to me, and I actually understood the gist of it before pushing my PR.

The only thing I propose at this point is to allow tests to run against the Guard master (I was modifying guard-rubocop and wanted test coverage without thinking about the ecosystem).

Anyway your PR is the way to go.