It's not clear that displaying
repr(Selector)is more readable for users than displaying
repr(Node). Is there a specific case that you were trying to make more readable?
More generally, I'm uncomfortable with the fact that there would now be a bunch of redundant data in the Node's identity, because any information in the Selector will already have been in the Node's identity, and is now in there in a second form.
This doesn't seem to be testing anything useful... the output is created by
namedtuple, so this is effectively a test of
[engine] Pass selectors to select nodes; Use selectors in error messages
Review Request #4031 — Created July 18, 2016 and submitted
|jsirois, patricklaw, stuhood, zundel|
Currently selectors are dropped when select nodes for task nodes are instantiated. This means that if a selector fails to match, the error message can't reproduce the selector.
This changes the select nodes so that they are passed the selector and can reuse it for producing error messages. It additionally overrides the
__repr__implementations for selectors so they'll be displayed similarly to how they were declared.
Currently a select failure will look like this:Noop(msg=u"No source of SelectNode(subject=PathGlobs(dependencies=(PathDirWildcard(canonical_stat=Dir(path=u''), symbolic_path=u'', wildcard=u'fs_test', remainder=u'a/b/*'),)), product=<class 'pants_test.engine.test_isolated_process.Concatted'>, variants=None, variant_key=None).").
With the repr and message changes, it looks like this:Noop(msg=u"No source of SelectNode(subject=PathGlobs(dependencies=(PathDirWildcard(canonical_stat=Dir(path=u''), symbolic_path=u'', wildcard=u'fs_test', remainder=u'a/b/*'),)), variants=None, selector=Select(Concatted)).")
CI away on PR.
Revert noop message; update description; remove redundant fields in SelectNode.
Revision 2 (+171 -65)
Thanks, this looks much better.
I think that one thing that continues to make me uncomfortable is that not all Nodes have Selectors (as implemented). How a
FilesystemNodeended up selected is a hole in the improved debugging that this offers.
One case that I think exists is that in some cases a Node can be instantiated from different Selectors (SelectLiteral/Select come to mind). So a Selector isn't (currently) a Node property so much as an "edge" property.
Also, it seems like it might be possible to clearly distinguish between intrinsic Nodes (those without Selectors, I think?) and user declared Nodes, and that might clean up the
Nodesubclasses in some way.