New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve type checking #872

Merged
merged 3 commits into from Jan 8, 2018

Conversation

Projects
None yet
3 participants
@ioquatix
Member

ioquatix commented Jun 15, 2017

(proposal) If you prefer to use a module for your plugin, this will break, and it's annoying to work around.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jun 15, 2017

Member

Another option is to use klass.ancestors.include?(Guard).

Member

ioquatix commented Jun 15, 2017

Another option is to use klass.ancestors.include?(Guard).

ioquatix added a commit to socketry/guard-falcon that referenced this pull request Jun 15, 2017

@thibaudgg

This comment has been minimized.

Show comment
Hide comment
@thibaudgg

thibaudgg Jun 16, 2017

Member

klass.ancestors.include?(Guard) seems safer to me, could you use that instead? 👍

Member

thibaudgg commented Jun 16, 2017

klass.ancestors.include?(Guard) seems safer to me, could you use that instead? 👍

@@ -55,7 +55,7 @@ def initialize(name)
def initialize_plugin(options)
klass = plugin_class
fail "Could not load class: #{_constant_name.inspect}" unless klass
if klass.superclass.to_s == "Guard::Guard"

This comment has been minimized.

@rymai

rymai Jun 17, 2017

Member

@ioquatix This is to support legacy Guard plugins that inherits from Guard::Guard, I don't see how klass.superclass.eql?(Guard) or klass.ancestors.include?(Guard) improves the situation if you're plugin is a module?

@rymai

rymai Jun 17, 2017

Member

@ioquatix This is to support legacy Guard plugins that inherits from Guard::Guard, I don't see how klass.superclass.eql?(Guard) or klass.ancestors.include?(Guard) improves the situation if you're plugin is a module?

This comment has been minimized.

@ioquatix

ioquatix Jun 17, 2017

Member

Because Module doesn't have #superclass so it breaks. klass needs to be an instance of Class or (as I've done) a module with an empty def self.superclass.

@ioquatix

ioquatix Jun 17, 2017

Member

Because Module doesn't have #superclass so it breaks. klass needs to be an instance of Class or (as I've done) a module with an empty def self.superclass.

This comment has been minimized.

@rymai

rymai Jun 19, 2017

Member

Oh I see, makes sense!

@rymai

rymai Jun 19, 2017

Member

Oh I see, makes sense!

@rymai

This comment has been minimized.

Show comment
Hide comment
@rymai

rymai Jun 19, 2017

Member

@ioquatix Specs are failing:

  1) Guard::PluginUtil#initialize_plugin with a plugin inheriting from Guard::Plugin instantiate the plugin using the new API
     Failure/Error: expect(guard_rspec_class).to receive(:superclass) { ::Guard::Plugin }
     
       (ClassDouble(Guard::Plugin) (anonymous)).superclass(*(any args))
           expected: 1 time with any arguments
           received: 0 times with any arguments
     # ./spec/lib/guard/plugin_util_spec.rb:73:in `block (4 levels) in <top (required)>'
Member

rymai commented Jun 19, 2017

@ioquatix Specs are failing:

  1) Guard::PluginUtil#initialize_plugin with a plugin inheriting from Guard::Plugin instantiate the plugin using the new API
     Failure/Error: expect(guard_rspec_class).to receive(:superclass) { ::Guard::Plugin }
     
       (ClassDouble(Guard::Plugin) (anonymous)).superclass(*(any args))
           expected: 1 time with any arguments
           received: 0 times with any arguments
     # ./spec/lib/guard/plugin_util_spec.rb:73:in `block (4 levels) in <top (required)>'
@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jun 19, 2017

Member

Yeah, I know, I saw that. It's a spec which specifically checks to see if superclass was called. It's not checking for the correct outcome, but some internal detail of how it works. Not sure - should we just remove it or rework it?

Member

ioquatix commented Jun 19, 2017

Yeah, I know, I saw that. It's a spec which specifically checks to see if superclass was called. It's not checking for the correct outcome, but some internal detail of how it works. Not sure - should we just remove it or rework it?

@rymai

This comment has been minimized.

Show comment
Hide comment
@rymai

rymai Jan 6, 2018

Member

Not sure - should we just remove it or rework it?

@ioquatix Sorry for the delay! I think we should rework it.

Member

rymai commented Jan 6, 2018

Not sure - should we just remove it or rework it?

@ioquatix Sorry for the delay! I think we should rework it.

@rymai rymai added this to the v2.15 milestone Jan 6, 2018

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jan 6, 2018

Member

Okay good I will review the code again.

Member

ioquatix commented Jan 6, 2018

Okay good I will review the code again.

ioquatix added some commits Jun 15, 2017

Improve type checking
(proposal) If you prefer to use a module for your plugin, this will break, and it's annoying to work around.
@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jan 7, 2018

Member

Okay, it's done. I rebased on current master too.

Member

ioquatix commented Jan 7, 2018

Okay, it's done. I rebased on current master too.

@rymai

This comment has been minimized.

Show comment
Hide comment
@rymai

rymai Jan 8, 2018

Member

@ioquatix Thank you, looks good to me! ❤️

Member

rymai commented Jan 8, 2018

@ioquatix Thank you, looks good to me! ❤️

@rymai rymai merged commit adcf25e into master Jan 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@rymai rymai deleted the plugin-improved-type-checking branch Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment