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

Support kw=Collection(task) in addition to Collection('kw', task) #528

Closed
tuukkamustonen opened this Issue May 21, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@tuukkamustonen

tuukkamustonen commented May 21, 2018

Pre-0.23 supported:

ns = Collection(
    env=Collection(some_task1, some_task2),
)

But now it fails (on py3.5 at least) to:

Traceback (most recent call last):
  File "/home/musttu/.pyenv/versions/his/bin/inv", line 11, in <module>
    sys.exit(program.run())
  File "/home/musttu/.pyenv/versions/3.5.5/envs/his/lib/python3.5/site-packages/invoke/program.py", line 325, in run
    self.parse_cleanup()
  File "/home/musttu/.pyenv/versions/3.5.5/envs/his/lib/python3.5/site-packages/invoke/program.py", line 450, in parse_cleanup
    self.list_tasks()
  File "/home/musttu/.pyenv/versions/3.5.5/envs/his/lib/python3.5/site-packages/invoke/program.py", line 665, in list_tasks
    getattr(self, "list_{}".format(self.list_format))()
  File "/home/musttu/.pyenv/versions/3.5.5/envs/his/lib/python3.5/site-packages/invoke/program.py", line 668, in list_flat
    pairs = self._make_pairs(self.scoped_collection, ancestors=[])
  File "/home/musttu/.pyenv/versions/3.5.5/envs/his/lib/python3.5/site-packages/invoke/program.py", line 739, in _make_pairs
    recursed_pairs = self._make_pairs(subcoll, ancestors + [coll])
  File "/home/musttu/.pyenv/versions/3.5.5/envs/his/lib/python3.5/site-packages/invoke/program.py", line 685, in _make_pairs
    ancestor_path = '.'.join(x.name for x in display_ancestors)
TypeError: sequence item 0: expected str instance, NoneType found

Now we have to give the namespace as str parameter to Collection as in:

ns = Collection(
    Collection('env', some_task1, some_task2),
)

Was this change intended or accidental? I liked to old way more (rather kwarg than positional string arg).

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 19, 2018

That sounds accidental to me. Based on the traceback I'm betting it has to do with the overhaul for --list; the collection itself may be instantiating okay and it's something more subtle causing the bug.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 19, 2018

Suspect this is missed in our test suite because the fixtures I use for testing --list (again, esp after the overhaul) all rely on the pattern where subcollections typically get their name from the module they're part of.

I can confirm we have a test about the actual kwarg-key-is-the-name behavior and it's passing, so this is either limited to --list only, or it's again more subtle.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 19, 2018

Cool, when I tweak that test fixture I'm getting a big bunch of errors all resembling yours or with same presumed root (a None instead of a string). Thanks for the report...patch should be in soon.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 19, 2018

Think this is due to an annoying modeling discrepancy: the fact that a task or collection can be added to a Collection multiple times, and thus there are two ways to "name" these objects - the dictionary key they are attached as at a given point, and/or their own internal/intrinsic name.

Most of the time, namespace-crawling functionality is concerned only with the former: something is requesting lookup of eg env.some_task, the root ns goes "ok who in me is env, oh it's this fella", then env is asked for some_task and goes "ah yes I have that key, here is the task object".

This is why, for example, in @tuukkamustonen's case, actually invoking env.some_task is working fine (at least, it is in my test cases).

However, the new list functionality at one point looks at the latter name - the object's intrinsic name - and in the case under examination, it's None (because, as Tuukka stumbled upon, that's driven entirely by whether the inner Collection was given a name argument or first posarg).


The naive fix is to modify the object when it's being given as a kwarg value - but this is misleading because as noted above, that's a big no-no (mutating an object you were given by your caller) in general. And in specifics, it would be problematic for us if the object were to be attached elsewhere in the tree later (the name would just get overwritten a second time).

So I'm 99% sure the right fix is to not reference .name in --list but to get it from the context somehow. Hopefully this is feasible...will check.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 20, 2018

Have that mostly solved but there is another minor add-on problem (I think more an issue with my test than reality), the now-not-a-module subcollection I tweaked lost its docstring, because...I'm handing in the raw collection and not the module.

Technically I should fix this by updating my test output assertions to not expect help text for that particular subcollection, but it makes me realize that there is no way to specify the help for a collection this way - the help functionality has been written assuming one is using module trees.

I'll probably hack it up now to save myself some work, but it means there needs to be a spinoff feature ticket.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 21, 2018

This is fixed now on master. Doing some unrelated release oriented tasks now but I'll push to GH soon. PyPI release should be within a day or two, hopefully.

@bitprophet bitprophet closed this Jun 21, 2018

bitprophet added a commit that referenced this issue Jun 22, 2018

bitprophet added a commit that referenced this issue Jun 22, 2018

Fix #528 by focusing on subcollections' bound names instead of intrin…
…sic names

Also happens to end up simplifying the overall recursion loop too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment