Add internal routines to remove args and targets #4714
Conversation
mwichmann
added
the
args_and_options
Adds SCons.Script._Remove_Target and SCons.Script._Remove_Argument to allow taking away values placed the public attributes BUILD_TARGETS, COMMAND_LINE_TARGETS, ARGUMENTS and ARGLIST. Part two of three harvesting from old PR SCons#3799 (the short-option piece was merged as part of SCons#4598). Intended customer will be the Options logic, Unit tests created, also for existing SCons.Script._Add_Targets and SCons.Script._Add_Arguments. Signed-off-by: Mats Wichmann <[email protected]>
mwichmann
force-pushed
the
feature/remove_args_targs
branch
from
f56175b to
7015496
Compare
|
Found a niggly error in the docstrings (I misspelled |
| @@ -30,16 +30,16 @@ | |||
| some other module. If it's specific to the "scons" script invocation, | |||
| it goes here. | |||
| """ | |||
|
|
|||
There was a problem hiding this comment.
this section is just a sort, not a change. It will never be possible to make checkers entirely happy here, because there's some code that has to run before some imports to get things right. Didn't feel like tagging every single import following the code bits, just did one that's well separated from the general import section (see line 138)
| ARGLIST.remove((a, b)) | ||
|
|
||
| # Remove first in case no matching values left in ARGLIST | ||
| ARGUMENTS.pop(a, None) |
There was a problem hiding this comment.
Not sure I understand what this is doing? Is a an integer? I thought list's pop() to an int?
There was a problem hiding this comment.
Oh IC.. And used pop() so if it's not in ARGUMENTS it won't throw an exception?
I think the comment is what's throwing me off.
Should be something like
# Now delete the argument from the ARGUMENTS dict
?
There was a problem hiding this comment.
This is the way it was written in #3799 - I can think about reword. It is a bit hokey - it's alluding to the need to possibly refill ARGUMENTS if there were other instances of the variable in ARGLIST (which I assume is an implementation choice we agree with?). I'd propose just removing the comment altogether, and slightly tweaking the following one.
Adds
SCons.Script._Remove_TargetandSCons.Script._Remove_Argumentto allow taking away values placed the public attributesBUILD_TARGETS,COMMAND_LINE_TARGETS,ARGUMENTSandARGLIST. Part two of three harvesting from old PR #3799 (the short-option piece was merged as part of #4598). Intended customer will be the Options logic,Unit tests created, also for existing
SCons.Script._Add_TargetsandSCons.Script._Add_Arguments.There is no public visibility, so no doc changes (except for docstrings for the API docs)
Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).