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

Aliases are inconsistent in Variables.options #4630

Open
dustdfg opened this issue Nov 11, 2024 · 7 comments
Open

Aliases are inconsistent in Variables.options #4630

dustdfg opened this issue Nov 11, 2024 · 7 comments
Labels
Variables Variables() subsystem

Comments

@dustdfg
Copy link

dustdfg commented Nov 11, 2024

Describe the bug
Aliases are inconsistent in Variables.options. Second member of tuple (which represents option) does contain original name if option doesn't have aliases. But it doesn't contain original name if option has aliases.

import SCons

env = Environment()

options = Variables()

options.Add(["option_with_alias", "alias", "alias2"], "", "")
options.Add("option_without_alias", "", "")

print(options)

Output:

Variables(
  options=[
    ('option_with_alias', ['alias', 'alias2'], '', '', validator=None, converter=None),
    ('option_without_alias', ['option_without_alias'], '', '', validator=None, converter=None),
  ],
  args={},
  files=[],
  unknown={},
)

Required information

  • Version of SCons 4.8.1
  • Version of Python 3.12.6
  • How you installed SCons 🤷‍♀️ maybe pip maybe apt from debian testing
@mwichmann
Copy link
Collaborator

agree, it's inconsistent. when I found that, we didn't want to change existing behavior if nobody was complaining. which version feels more natural? name in aliases or not?

@dustdfg
Copy link
Author

dustdfg commented Nov 11, 2024

Today I needed to implement custom UnknownVariables 1 . I needed to manually check if something provided bu user is inside env or opts. And failed with env because env["alias"] as like "alias" in env failed with KeyError. Ok... I though... Env is env a n object which is used for actual process while Variables object generally is ruleset so ok lets check inside Variables and found this inconsistency... In my case it would be easier if I have ("name" ["name" + aliases] but from perspective of general logic it is better to have empty aliases array for option without aliases to separate them 🤷‍♀️

Footnotes

  1. because it is broken too. It doesn't list variables defined inside files (because I read code and IIUC it pushes to unknown list only options which came from cli). Variables in general is too restrictive... It doesn't allow you to easily distinguish what came from files and what from cli args and etc...

@dustdfg
Copy link
Author

dustdfg commented Nov 11, 2024

agree, it's inconsistent. when I found that, we didn't want to change existing behavior if nobody was complaining

Do you need someone to complain? I am ready to complain! I've already seen tons of workarounds and needed to create them myself. I only work with scons for 1~1.5 months... From now I will report everything I find

@dustdfg
Copy link
Author

dustdfg commented Nov 11, 2024

Or is it a published api? If no, then it will hurt many people (I will be hurted too), but there is option better. Tuples doesn't have embedded names so is there any difference to do options.options[0] to get actual name or do options.options[0][0]? I propose here to merge these two options so if you doesn't have aliases you will get (["name"] and if you have aliases you will get (["name", "alias1", "alias2"]

@dustdfg
Copy link
Author

dustdfg commented Nov 11, 2024

And just formally as option itself is a tuple, why not to use tuple instead of list for aliases? Does SCons allow to modify aliases at runtime? If no why not to use tuple? More constness

@mwichmann mwichmann added the Variables Variables() subsystem label Nov 12, 2024
@mwichmann
Copy link
Collaborator

@bdbaddog any opinions here? When I ran across this before, I left it as-is, because tests were written to the existing behavior, and I left a comment about it. I didn't remember, but it looks like I preferred the eventual approach of leaving the name out of the alias list in both cases.

        if SCons.Util.is_Sequence(key):
            option.key = key[0]
            option.aliases = list(key[1:])
        else:
            option.key = key
            # TODO: normalize to not include key in aliases. Currently breaks tests.
            option.aliases = [key,]

The name (key) is added later for processing anyway, so it doesn't really need to be there, e.g. like this:

                if arg in option.aliases + [option.key,]:

I don't have an opinion on tuple-vs-list, which is a separate question above.

@bdbaddog
Copy link
Contributor

agree, it's inconsistent. when I found that, we didn't want to change existing behavior if nobody was complaining

Do you need someone to complain? I am ready to complain! I've already seen tons of workarounds and needed to create them myself. I only work with scons for 1~1.5 months... From now I will report everything I find

Please don't just file issue without discussing them first. To ensure they are a) actually a issue and not user error, and b) that there isn't already an issue filed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Variables Variables() subsystem
Projects
None yet
Development

No branches or pull requests

3 participants