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

Making sure all Commands use run in __call__. #48

Merged
merged 1 commit into from Sep 18, 2017

Conversation

dhermes
Copy link
Collaborator

@dhermes dhermes commented Sep 18, 2017

This way subclasses don't have to also set __call__ = run, they can just over-ride run.

ASIDE 1: @jonparrott Do you want me to write docstrings?
ASIDE 2: @jonparrott Is there a reason InstallCommand does not subclass Command?
ASIDE 3: I would label this as hygiene, but the label doesn't exist.

This way subclasses don't have to also set `__call__ = run`,
they can just over-ride `run`.
@theacodes
Copy link
Collaborator

ASIDE 1: @jonparrott Do you want me to write docstrings?

No need to write trivial docstrings (this is not a user-facing API)

ASIDE 2: @jonparrott Is there a reason InstallCommand does not subclass Command?

Probably just an oversight. Wanna see if inheriting breaks anything?

@dhermes
Copy link
Collaborator Author

dhermes commented Sep 18, 2017

Probably just an oversight. Wanna see if inheriting breaks anything?

Done and pushed.

@dhermes
Copy link
Collaborator Author

dhermes commented Sep 18, 2017

ISTM it will make Pylint angry that the constructor doesn't call super().

@theacodes
Copy link
Collaborator

ISTM it will make Pylint angry that the constructor doesn't call super().

I see - undo that for now, I think we just need to revisit and make the base class more abstract.

@dhermes
Copy link
Collaborator Author

dhermes commented Sep 18, 2017

@jonparrott Undid it. Unclear why the AppVeyor build is failing (it's the same error message I saw on Travis when updating InstallCommand(object) -> InstallCommand(Command)).

@theacodes theacodes merged commit 7b37dc6 into wntrblm:master Sep 18, 2017
@dhermes dhermes deleted the composable branch September 18, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants