From 677633581e642708879eea7d7127fda6814509a0 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Mon, 25 Nov 2024 13:23:09 -0700 Subject: [PATCH 1/2] Turn SCons.Variables.Variable into a dataclass Also correct one long-standing minor inconsistency: if a list of names for a variable is given, it is split into name and aliases, with the name not duplicated into the aliases. If only one name is given, it is both the name and the only entry in the aliases. All of the usages of aliases combine them back together anyway, like: if arg in option.aliases + [option.key,]: So there's no need for the behavior of the one-name form including the name in aliases. One test did need to change for this - the test of a custom help formatter function was wired to accept the old form, but only because of the way the custom formatter in the test was written, so this is a self-contained problem. Signed-off-by: Mats Wichmann --- CHANGES.txt | 7 ++++ RELEASE.txt | 5 +++ SCons/Variables/VariablesTests.py | 14 ++++---- SCons/Variables/__init__.py | 56 ++++++++++++------------------- 4 files changed, 40 insertions(+), 42 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index aa29180f5..155ab6b56 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -167,6 +167,13 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER their values taken from the default in the variable description (if a variable was set to the same value as the default in one of the input sources, it is not included in this list). + - If a build Variable is created with no alises, the name of the + Variable is no longer listed in its aliases. Internally, the name + and alises are considered together anyway so this should not have + any effect except for being visible to custom help text formatters. + - A build Variable is now a dataclass, with initialization moving to + the automatically provided method; the Variables class no longer + writes directly to a Variable (makes static checkers happier). RELEASE 4.8.1 - Tue, 03 Sep 2024 17:22:20 -0700 diff --git a/RELEASE.txt b/RELEASE.txt index aa3e3c15d..94ffa0c10 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -82,6 +82,11 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY (if a variable was set to the same value as the default in one of the input sources, it is not included in this list). +- If a build Variable is created with no alises, the name of the + Variable is no longer listed in its aliases. Internally, the name + and alises are considered together anyway so this should not have + any effect except for being visible to custom help text formatters. + FIXES ----- diff --git a/SCons/Variables/VariablesTests.py b/SCons/Variables/VariablesTests.py index bc981e06f..c409db402 100644 --- a/SCons/Variables/VariablesTests.py +++ b/SCons/Variables/VariablesTests.py @@ -550,7 +550,7 @@ def my_format(env, opt, help, default, actual, aliases) -> str: check, lambda x: int(x) + 12) - opts.Add('B', + opts.Add(['B', 'BOPTION'], 'b - alpha test', "42", check, @@ -566,9 +566,9 @@ def my_format(env, opt, help, default, actual, aliases) -> str: opts.Update(env, {}) expect = """\ -ANSWER 42 54 THE answer to THE question ['ANSWER'] -B 42 54 b - alpha test ['B'] -A 42 54 a - alpha test ['A'] +ANSWER 42 54 THE answer to THE question [] +B 42 54 b - alpha test ['BOPTION'] +A 42 54 a - alpha test [] """ text = opts.GenerateHelpText(env) @@ -576,9 +576,9 @@ def my_format(env, opt, help, default, actual, aliases) -> str: self.assertEqual(expect, text) expectAlpha = """\ -A 42 54 a - alpha test ['A'] -ANSWER 42 54 THE answer to THE question ['ANSWER'] -B 42 54 b - alpha test ['B'] +A 42 54 a - alpha test [] +ANSWER 42 54 THE answer to THE question [] +B 42 54 b - alpha test ['BOPTION'] """ text = opts.GenerateHelpText(env, sort=cmp) with self.subTest(): diff --git a/SCons/Variables/__init__.py b/SCons/Variables/__init__.py index de26e7b65..521380468 100644 --- a/SCons/Variables/__init__.py +++ b/SCons/Variables/__init__.py @@ -28,8 +28,9 @@ import os.path import sys from contextlib import suppress +from dataclasses import dataclass from functools import cmp_to_key -from typing import Callable, Sequence +from typing import Any, Callable, Sequence import SCons.Errors import SCons.Util @@ -53,22 +54,18 @@ "PathVariable", ] + +@dataclass(order=True) class Variable: """A Build Variable.""" - __slots__ = ('key', 'aliases', 'help', 'default', 'validator', 'converter', 'do_subst') - - def __lt__(self, other): - """Comparison fuction so :class:`Variable` instances sort.""" - return self.key < other.key - - def __str__(self) -> str: - """Provide a way to "print" a :class:`Variable` object.""" - return ( - f"({self.key!r}, {self.aliases}, " - f"help={self.help!r}, default={self.default!r}, " - f"validator={self.validator}, converter={self.converter})" - ) + key: str + aliases: list[str] + help: str + default: Any + validator: Callable | None + converter: Callable | None + do_subst: bool class Variables: @@ -131,7 +128,7 @@ def __str__(self) -> str: # lint: W0622: Redefining built-in 'help' def _do_add( self, - key: str | list[str], + key: str | Sequence[str], help: str = "", default=None, validator: Callable | None = None, @@ -146,30 +143,19 @@ def _do_add( .. versionadded:: 4.8.0 *subst* keyword argument is now recognized. """ - option = Variable() - - # If we get a list or a tuple, we take the first element as the - # option key and store the remaining in aliases. + # aliases needs to be a list for later concatenation operations if SCons.Util.is_Sequence(key): - option.key = key[0] - option.aliases = list(key[1:]) + name, aliases = key[0], list(key[1:]) else: - option.key = key - # TODO: normalize to not include key in aliases. Currently breaks tests. - option.aliases = [key,] - if not option.key.isidentifier(): - raise SCons.Errors.UserError(f"Illegal Variables key {option.key!r}") - option.help = help - option.default = default - option.validator = validator - option.converter = converter - option.do_subst = kwargs.pop("subst", True) - # TODO should any remaining kwargs be saved in the Variable? - + name, aliases = key, [] + if not name.isidentifier(): + raise SCons.Errors.UserError(f"Illegal Variables key {name!r}") + do_subst = kwargs.pop("subst", True) + option = Variable(name, aliases, help, default, validator, converter, do_subst) self.options.append(option) - # options might be added after the 'unknown' dict has been set up, - # so we remove the key and all its aliases from that dict + # options might be added after the 'unknown' dict has been set up: + # look for and remove the key and all its aliases from that dict for alias in option.aliases + [option.key,]: if alias in self.unknown: del self.unknown[alias] From 8558c8f9528fad24a86996d7ec97750b817cd5ee Mon Sep 17 00:00:00 2001 From: William Deegan Date: Wed, 11 Dec 2024 18:29:42 -0800 Subject: [PATCH 2/2] [ci skip] fix typo in CHANGES/RELEASE --- CHANGES.txt | 4 ++-- RELEASE.txt | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 155ab6b56..d73da372e 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -167,9 +167,9 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER their values taken from the default in the variable description (if a variable was set to the same value as the default in one of the input sources, it is not included in this list). - - If a build Variable is created with no alises, the name of the + - If a build Variable is created with no aliases, the name of the Variable is no longer listed in its aliases. Internally, the name - and alises are considered together anyway so this should not have + and aliases are considered together anyway so this should not have any effect except for being visible to custom help text formatters. - A build Variable is now a dataclass, with initialization moving to the automatically provided method; the Variables class no longer diff --git a/RELEASE.txt b/RELEASE.txt index 94ffa0c10..0838c6973 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -82,9 +82,9 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY (if a variable was set to the same value as the default in one of the input sources, it is not included in this list). -- If a build Variable is created with no alises, the name of the +- If a build Variable is created with no aliases, the name of the Variable is no longer listed in its aliases. Internally, the name - and alises are considered together anyway so this should not have + and aliases are considered together anyway so this should not have any effect except for being visible to custom help text formatters. FIXES