[WIP] Use only one NodeList definition #3992
Conversation
The two definitions of NodeList are collapsed to one, keeping the SCons.Util version and dumping the SCons.Node one. Two unit tests that were expecting the __str__ behavior of SCons.Node.NodeList were adjusted to match the behavior of SCons.Util.NodeList. Alias() now returns NodeList: the list of nodes generated by calling Alias is converted to a NodeList before returning. This brings it into line with the claim "builders return a nodelist". Some type annotation added re: NodeLists, and based on running mypy, some other adjustments made. Signed-off-by: Mats Wichmann <mats@linux.com>
The initial commit removed NodeList's __getitem__ method, since it's no longer needed. Turned out the fix was only applied in cpython as of the 3.7 release, so put it back for versions earlier than that. Also add a __getitem__ (versioned the same way) to CLVar class, to make sure it keeps its identity if sliced. Added unit tests to make sure slicing these two UserList derivatives retains the class. Signed-off-by: Mats Wichmann <mats@linux.com>
| From Mats Wichmann: | ||
| - Unify the NodeList definitions - there were two, and didn't need to be. | ||
| Two unit tests had to be updated to make sure they're expecting the | ||
| right string representation. Alias now returns a NodeList like other |
There was a problem hiding this comment.
Probably should explain the change to str(NodeList) here in case someone is depending on it.
Also.. they could cause unnecessary rebuilds on upgrade if the str(NodeList) is used anywhere generating a sig.
Also should list all the methods/apis which now return a NodeList() when before they returned a list [].
Lastly.. any idea if this has any notable perf degradation?
There was a problem hiding this comment.
Would have to write somethng synthetic to check the performance, nothing we have in the testsuite takes enough time in NodeLists to show any significant difference. (back-to-back test runs show the new way a few seconds faster, but that's probably just within the noise.
I'll queue up trying to list the changes. In general following the "builders return a NodeList" rule, even for things which aren't classically builders (Builder classes) but are documented as builders.
There was a problem hiding this comment.
as a first step, I've added some information in the PR description above (see Edited section).
| SConscript.BuildTargets = BUILD_TARGETS | ||
| SConscript.CommandLineTargets = COMMAND_LINE_TARGETS | ||
| SConscript.DefaultTargets = DEFAULT_TARGETS | ||
| # that pychecker and mypy don't barf. |
There was a problem hiding this comment.
the checkers don't like the previous syntax?
There was a problem hiding this comment.
Correct. They gripe that there's no such attribute. Obviously Python allows you to add them this way, but it seemed a cheap change to shut the tools up.
| # Return one item of the tart | ||
| return self.data[index] | ||
| # Return one item of the tart | ||
| return self.data[index] |
There was a problem hiding this comment.
no __getitem__ needed for py >= 3.7 ?
There was a problem hiding this comment.
The inherited implementation from UserList works correctly, as noted in the referenced issue.
There was a problem hiding this comment.
apparently that had been a place that wasn't fully fixed up when Py3 dropped the previously existing __slice__ method.
Signed-off-by: Mats Wichmann <mats@linux.com>
Signed-off-by: Mats Wichmann <mats@linux.com>
|
I'd drop the |
|
Ok. Was that kind of list useful, or were you looking for something further? |
Yes. that's what I was looking for, but remove |
Clarify some bits of Alias while adding the mention it returns a NodeList. Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann
changed the title
|
Flipped to WIP while I research some things. |
mwichmann
changed the title
|
I'm now inclined to withdraw this one, maybe come back someday with a new approach. Any objections? |
|
I'm going to withdraw this one. I still have questions, and haven't had time to research in detail, whether this is worth doing - perhaps the irritation of two of these is just something we can live with. Will keep archived. |
The two definitions of
NodeListare collapsed to one, keeping theSCons.Utilversion and dumping theSCons.Nodeone. Two unit tests that were expecting the__str__behavior ofSCons.Node.NodeListwere adjusted to match the behavior ofSCons.Util.NodeList.Alias()now returnsNodeList. This brings it into line with the claim "builders return a nodelist".Some type annotation added re:
NodeLists, and based on runningmypy, some other adjustments made.Edited:
The Node
NodeList__str__method produces a string which begins and ends with brackets, the Util one does not. I don't have a strong opinion which is better, but the unit tests of course expect a very specific behavior. It's unclear if the str change affects anything else (investigate).This code changes the
_executemethod to return aSCons.Util.NodeListinstead of anSCons.Node.NodeList. The main one is inBuilderBase, but some derived classes define their own, which are currently_PDFFileBuilderinGettextCommon.py,_POTBuilderinxgettext.pyand_MDFileBuilderinmsgfmt.py.The following public methods now explcitly return a
NodeList, where previously some did and some returned plain Python lists:AddPreAction,AddPostAction,Alias,AlwaysBuild,Command,Depends,NoClean,NoCache,Ignore,Precious,Pseudo,Requires,SideEffect.These internal methods methods now return a
NodeListwhere they previously returned a Python list:arg2nodes,_create_nodes.Signed-off-by: Mats Wichmann mats@linux.com
Contributor Checklist:
CHANGES.txt(and read theREADME.rst)