Skip to content
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

fix and test for issue 1365: exception subclasses should not be recog… #1372

Closed
wants to merge 1 commit into from

Conversation

@mattvonrocketstein
Copy link

@mattvonrocketstein mattvonrocketstein commented Sep 3, 2015

…nized as fabric commands

…nized as fabric commands
@@ -125,6 +126,7 @@ def is_classic_task(tup):
callable(func)
and (func not in _internals)
and not name.startswith('_')
and not (inspect.isclass(func) and issubclass(func, Exception))

This comment has been minimized.

@kaaveland

kaaveland Dec 16, 2015

I'm wondering to myself if it makes sense that any class is a task? I'm wondering if maybe we should just refuse classes. Suppose I want to generate some csv from a bunch of servers and I put the following in my file: from csv import CsvWriter, Dialect -- with current master, fabric will list Dialect Describe an Excel dialect. in fab -l.

This comment has been minimized.

@mattvonrocketstein

mattvonrocketstein Dec 16, 2015
Author

I think what you say is totally reasonable, and would be all for excluding all classes and not just exception subclasses.

FWIW, I made this change for exceptions only because it seemed like a good combination of useful/safe. It's useful because, for quick fabfiles, I'm betting exceptions are imported more often than classes. It's safe because even though there's probably one maniac out there that has some crazy work flow that depends on the existing fabfile behaviour with respect to classes, surely no one is counting on the existing Exception behaviour, right?

This comment has been minimized.

@kaaveland

kaaveland Dec 17, 2015

Fair enough, I do agree that some crazy person could be using __init__ as a task and I agree this seems less likely to break for anyone.

This comment has been minimized.

@bitprophet

bitprophet Dec 17, 2015
Member

Class-based tasks are actually a definite (if minority) use case, so the limitation of "it's an Exception" should stay, IMHO.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Dec 17, 2015

Dropping #1365 here for linkage.

@bitprophet bitprophet added this to the 1.10.3 milestone Dec 17, 2015
@bitprophet bitprophet added the Bug label Dec 17, 2015
bitprophet added a commit that referenced this pull request Apr 7, 2016
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 7, 2016

Cherry-picked to 1.10 branch as 4e1fd83 - closing & releasing soon, thanks!

@bitprophet bitprophet closed this Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.