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

Added list to core_args and added check for no-dedup. Program was ass… #288

Merged
merged 1 commit into from Nov 30, 2016

Conversation

jordoncm
Copy link
Contributor

…uming these would be a part of the flag structure which breaks creation of a Program.run where a namespace is passed in.

Consider the self contained python script ./foo

#!/usr/bin/env python

import sys

import invoke

@invoke.task
def build():
  print 'built!'

if __name__ == '__main__':
  namespace = invoke.Collection()
  namespace.add_task(build)
  program = invoke.Program(
    name='Foo',
    binary='foo',
    version='0.1',
    namespace=namespace
  )
  program.run(sys.argv)

invoke.Program would except because it assumes self.args.list was defined. The list operation makes sense to be a part of core_args in this use case. After fixing this invoke.Program excepts again assuming self.args['no-dedup'] is defined.

…uming these would be a part of the flag structure which breaks creation of a Program.run where a namespace is passed in.
@bitprophet
Copy link
Member

Thanks for the catch. I assume I chopped up the core-args vs non-core-args stuff that way on purpose, so need to doublecheck the tickets & git history for it to be sure this won't cause another problem. But could've just been an oversight.

@jordoncm
Copy link
Contributor Author

The other solution to this is to not assume self.args.list is present here (possibly elsewhere too): https://github.com/pyinvoke/invoke/blob/master/invoke/program.py#L341

@bitprophet
Copy link
Member

I assume I chopped up the core-args vs non-core-args stuff that way on purpose

Revisiting this now that it's biting me in my dayjob use - I can see why I initially thought "don't expose --list, we're pretending to be a Real Program with Real Subcommands". However, I myself attempted to do custombinary --list recently, implying it's intuitive if you're used to Invoke itself.

Given that I hate unintuitive things and it's not super likely that non-Invoke users will be bothered by its existence...and of course given that having it in the non-core args is causing problems...I think probably OK to shuffle it around for now, as per this PR.

@bitprophet
Copy link
Member

Also, @sophacles made the same argument re: inclusion of list in this comment some time ago (re #276). So one less thing for that branch to handle whenever I get back to it (sob).

@bitprophet
Copy link
Member

We don't seem to cover the core bug this fixes in either the unit or integration test suites, sadface. Will whip one up.

@bitprophet bitprophet merged commit e6f77fc into pyinvoke:master Nov 30, 2016
bitprophet added a commit that referenced this pull request Nov 30, 2016
@bitprophet
Copy link
Member

All set + tests + changelog, thanks again!

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

Successfully merging this pull request may close these issues.

None yet

2 participants