Skip to content
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

Add a routine to remove entries from a construction variable #4332

Open
mwichmann opened this issue Mar 29, 2023 · 2 comments
Open

Add a routine to remove entries from a construction variable #4332

mwichmann opened this issue Mar 29, 2023 · 2 comments
Labels
Append Issues in Append/Prepend + Unique variants

Comments

@mwichmann
Copy link
Collaborator

mwichmann commented Mar 29, 2023

The current SCons API has ways to add to construction variables with Append, AppendUnique, Prepend, PrependUnique, AppendENVPath, PrependENVPath and to directly assign to - Environment kwargs, ovverrides in builder calls, Replace. What is does not seem to include is a way to remove elements from a multi-element construction var. Resorting to pure Python syntax is not completely desirable, because then you have to know the type holding the construction variable to pick the right syntax. This has become a little more prominent now since 4.5 for CPPDEFINES, which, if it gets converted internally, will end up as a deque - a type people probably won't guess (we might still want to change that back to a plain list type) - but it's not actually a new problem, because the appending logic has always been able to do type conversions of its own choosing.

An initial thought is the syntax could look somewhat like a dictionary's pop method:

     D.pop(k[, d]) -> v, remove specified key and return the corresponding value.
    
    If the key is not found, return the default if given; otherwise,  raise a KeyError.

The "return the corresponding value" part isn't that essential, but list-oriented methods typically need an index to remove a value, and it feels like it would be a nicer to do the lookup for the user. CPPDEFINES, which always needs special case handling as its members may be containers, not just scalars, already has a "match" routine - _is_in, currently an inner function but which could be moved outside - and possibly be given an optional arg to remove the found entry.

Open to better suggestions for design (and name) of the interface...

@mwichmann mwichmann added the Append Issues in Append/Prepend + Unique variants label Mar 29, 2023
@mwichmann
Copy link
Collaborator Author

mwichmann commented Apr 6, 2023

Expanding a bit on the questions...

  • Could follow the model for sequences, which have a remove method matching by value (and don't return anything). Implies separate args for consvar and value to remove from it, since we don't have the object for the consvar to act on directly, have to get it first.
  • Could follow the model for mappings, which have a pop method matching by value, meaning the key name in this case (and return the existing value - but we asked for that value, so not sure what benefit there is in returning it). Implies separate args for consvar and value to remove from it, since we don't have the object for the consvar to act on directly, have to get it first (or see 4th item).
  • Between [].remove and {}.pop there's another interesting difference besides return/not-return: for the dict form, an optional argument to supply a default changes the behavior from exception if not found to return the value of default if not found, which implies don't raise exception
  • Could follow the model for env.Replace, which takes kwargs - i.e. can use cvar=removal form, and in this case could take multiple args, not just one. Implies it makes no sense to return, as what would you return for multiple removals (or... could be a tuple of successful removals, or maybe better a set of kwargs of cvar=removal). Also: how to handle errors if some work and some fail? exception on first-fail? keep going?
  • Would there be any benefit to also being able to remove the construction var entirely? Same API? If so, what syntax?

@mwichmann
Copy link
Collaborator Author

mwichmann commented Apr 11, 2023

After letting this stew for a bit... the last bullet is not needed, you can just del env['FOO']. And seems to me, copying the approach of Replace is the cleanest, that way don't have to stretch the mental model ("could be like X, but if the consvar is a different type, that's not an exact fit). So my proposal now would be:

env.Remove(missing_ok=False, **kwargs)

:: Remove values from construction variables. For each key=value pair, remove value from the construction variable named by key. If missing_ok is false (the default), raise an exception if, in any pair, value is not present in env[key]. If true, continue processing.

@mwichmann mwichmann changed the title Add a routine to "pop" from a construction variable Add a routine to remove entries from a construction variable Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Append Issues in Append/Prepend + Unique variants
Projects
None yet
Development

No branches or pull requests

1 participant