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

Projects
None yet
2 participants
@jordoncm
Contributor

jordoncm commented Dec 13, 2015

…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.

Added list to core_args and added check for no-dedup. Program was ass…
…uming these would be a part of the flag structure which breaks creation of a Program.run where a namespace is passed in.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 15, 2015

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

This comment has been minimized.

Contributor

jordoncm commented Dec 19, 2015

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

This comment has been minimized.

Member

bitprophet commented Nov 30, 2016

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

This comment has been minimized.

Member

bitprophet commented Nov 30, 2016

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

This comment has been minimized.

Member

bitprophet commented Nov 30, 2016

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

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bitprophet added a commit that referenced this pull request Nov 30, 2016

@bitprophet

This comment has been minimized.

Member

bitprophet commented Nov 30, 2016

All set + tests + changelog, thanks again!

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