Add internal routines to remove args and targets#4714
Conversation
mwichmann
added
the
args_and_options
mwichmann
force-pushed
the
feature/remove_args_targs
branch
from
f56175b to
7015496
Compare
|
Found a niggly error in the docstrings (I misspelled |
| 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.
There was a problem hiding this comment.
Okay, I'll see about doing an update.
There was a problem hiding this comment.
I've reworked the commentary. Had to force-push as branches were out of sync, so rebased RELEASE and CHANGES too.
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 <mats@linux.com>
Make it more clear why we're doing work to put a value back into ARGUMENTS after removing it.
mwichmann
force-pushed
the
feature/remove_args_targs
branch
from
9b1660d to
cd2b787
Compare
2 participants
Adds
SCons.Script._Remove_TargetandSCons.Script._Remove_Argumentto allow taking away values placed in the public attributesBUILD_TARGETS,COMMAND_LINE_TARGETS,ARGUMENTSandARGLIST. This can be important when we see anAddOptionwhich changes what we originally thought an argument word was (e.g.,--foo barwould originally be sorted as "unknown option--foo" and "targetbar". A declaration in an SConscript that--foois an option that takes one argument meansbarwas not a target after all).Part two of three harvested from old PR #3799 (the short-option part 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).