diff --git a/docs/source/contributing/conventions/registry-protocol.rst b/docs/source/contributing/conventions/registry-protocol.rst index 79477709a..f266ee4dd 100644 --- a/docs/source/contributing/conventions/registry-protocol.rst +++ b/docs/source/contributing/conventions/registry-protocol.rst @@ -231,11 +231,13 @@ precedent is :issue:`903`, which made what turned ``Method.get`` into a classmethod. Callers cannot see the switch -- ``Method.get('X')`` binds identically either way -- -so there is no compatibility argument for keeping the ``@staticmethod``. Two -``@staticmethod`` overrides do survive and are still correct: -:class:`~pcapkit.const.ftp.command.Command`'s and -:class:`~pcapkit.const.pcapng.option_type.OptionType`'s never call ``super()`` at -all, so neither meets the condition. +so there is no compatibility argument for keeping the ``@staticmethod``. +:class:`~pcapkit.const.ftp.command.Command`'s override made the same switch on +:issue:`1824`, once it delegated a non-``str`` key to the base instead of calling +``key.upper()`` on it. +:class:`~pcapkit.const.pcapng.option_type.OptionType`'s ``@staticmethod`` override +does survive and is still correct: it never calls ``super()`` at all, so it does not +meet the condition. **Raise the way the base raises, which means** ``quiet=True``. :issue:`933` asked whether two diff --git a/pcapkit/const/ftp/command.py b/pcapkit/const/ftp/command.py index f50baca74..b8ebc4037 100644 --- a/pcapkit/const/ftp/command.py +++ b/pcapkit/const/ftp/command.py @@ -570,19 +570,58 @@ def register_alias(cls, value: 'str', name: 'str') -> 'Command': """ return super().register_alias(value, name.upper()) - @staticmethod - def get(key: 'str', default: 'Optional[str]' = None) -> 'Command': + @classmethod + def get(cls, key: 'str', default: 'Optional[str]' = None) -> 'Command': """Backport support for original codes. + A non-:class:`str` ``key`` is delegated to + :meth:`~pcapkit.corekit.enum.EnumLookup.get`, as + :meth:`Method.get ` delegates + every key, so :meth:`_validate_value` refuses it there rather than + ``key.upper()`` being called on it (GitHub issue :issue:`1824`). It is + a :class:`classmethod`, as the base is, because a zero-argument + ``super()`` needs a first argument to bind. ``Command.get('X')`` binds + the same as it would on a :class:`staticmethod`. + + ``default`` is not forwarded on that path, as ``Method.get`` does not + forward its own: here it supplies the *value* of the unregistered + member built on a name miss, while the base's ``default`` must name a + *registered* value. A refused ``key`` therefore raises whatever + ``default`` is. + + .. versionchanged:: 1.5.0 + + A non-:class:`str` ``key`` raises + :exc:`~pcapkit.utilities.exceptions.EnumValueError`, with or + without ``default`` (:issue:`1824`). ``get(42)`` and ``get(None)`` + raised :exc:`AttributeError` from ``key.upper()``, and + ``get(b'ABOR')`` returned an unregistered member whose name and + value were both the :class:`bytes` object. + Args: - key: Key to get enum item. Looked up case-insensitively, since - member names are canonicalised to upper case on registration. - default: Default value if not found. + key: Key to get enum item. Looked up case-insensitively, as + :rfc:`959#section-5` treats FTP command codes, against member + names canonicalised to upper case on registration. + default: Value for the unregistered member built when ``key`` + matches no member name. ``None``, the default, uses ``key`` + itself. + + Raises: + EnumValueError: If ``key`` is not a :class:`str`. This reaches the + caller from the base's non-``str`` branch, where + :meth:`_validate_value` refuses it before ``cls(key)`` could + reach :meth:`_missing_`. A :exc:`ValueError`, so an + ``except ValueError`` caller is unaffected. :meta private: """ + if not isinstance(key, str): + # NOTE: unreachable only by the ``str`` annotation, which a caller + # at run time need not honour -- this is the path GitHub issue + # #1824 fixed. + return super().get(key) # type: ignore[unreachable] name = key.upper() - if name not in Command._member_map_: # type: ignore[misc] # pylint: disable=no-member + if name not in cls._member_map_: # pylint: disable=no-member # NOTE: the value is ``default`` if the caller supplied one, or # else ``key`` exactly as given -- never ``name`` -- so an # unregistered member's value is the caller's own casing, the @@ -595,8 +634,8 @@ def get(key: 'str', default: 'Optional[str]' = None) -> 'Command': # ``_member_map_`` cache, the *first* casing seen for every later # call regardless of case; that is the one observable behaviour # change in GitHub issue #860's conversion. - return Command._unregistered_member(default if default is not None else key, name) - return Command[name] # type: ignore[misc] + return cls._unregistered_member(default if default is not None else key, name) + return cls._member_map_[name] # pylint: disable=no-member @classmethod def _validate_value(cls, value: 'Any') -> 'None': @@ -605,11 +644,13 @@ def _validate_value(cls, value: 'Any') -> 'None': Overrides :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` with the check :meth:`_missing_` makes, and the message it raises, so :meth:`register` refuses a value the constructor refuses, in the same - words (:issue:`1816`). Any :class:`str` passes, since - :meth:`_missing_` resolves every one. + words (:issue:`1816`). :meth:`get` reaches it as well, through the + base, for every non-:class:`str` key (:issue:`1824`), so it refuses + the same values. Any :class:`str` passes, since :meth:`_missing_` + resolves every one. Args: - value: Value about to be minted. + value: Value about to be looked up or minted. Raises: EnumValueError: If ``value`` is not a :class:`str`. A diff --git a/pcapkit/vendor/ftp/command.py b/pcapkit/vendor/ftp/command.py index cdfb5f194..7897be905 100644 --- a/pcapkit/vendor/ftp/command.py +++ b/pcapkit/vendor/ftp/command.py @@ -404,19 +404,58 @@ def register_alias(cls, value: 'str', name: 'str') -> '{NAME}': """ return super().register_alias(value, name.upper()) - @staticmethod - def get(key: 'str', default: 'Optional[str]' = None) -> '{NAME}': + @classmethod + def get(cls, key: 'str', default: 'Optional[str]' = None) -> '{NAME}': """Backport support for original codes. + A non-:class:`str` ``key`` is delegated to + :meth:`~pcapkit.corekit.enum.EnumLookup.get`, as + :meth:`Method.get ` delegates + every key, so :meth:`_validate_value` refuses it there rather than + ``key.upper()`` being called on it (GitHub issue :issue:`1824`). It is + a :class:`classmethod`, as the base is, because a zero-argument + ``super()`` needs a first argument to bind. ``{NAME}.get('X')`` binds + the same as it would on a :class:`staticmethod`. + + ``default`` is not forwarded on that path, as ``Method.get`` does not + forward its own: here it supplies the *value* of the unregistered + member built on a name miss, while the base's ``default`` must name a + *registered* value. A refused ``key`` therefore raises whatever + ``default`` is. + + .. versionchanged:: 1.5.0 + + A non-:class:`str` ``key`` raises + :exc:`~pcapkit.utilities.exceptions.EnumValueError`, with or + without ``default`` (:issue:`1824`). ``get(42)`` and ``get(None)`` + raised :exc:`AttributeError` from ``key.upper()``, and + ``get(b'ABOR')`` returned an unregistered member whose name and + value were both the :class:`bytes` object. + Args: - key: Key to get enum item. Looked up case-insensitively, since - member names are canonicalised to upper case on registration. - default: Default value if not found. + key: Key to get enum item. Looked up case-insensitively, as + :rfc:`959#section-5` treats FTP command codes, against member + names canonicalised to upper case on registration. + default: Value for the unregistered member built when ``key`` + matches no member name. ``None``, the default, uses ``key`` + itself. + + Raises: + EnumValueError: If ``key`` is not a :class:`str`. This reaches the + caller from the base's non-``str`` branch, where + :meth:`_validate_value` refuses it before ``cls(key)`` could + reach :meth:`_missing_`. A :exc:`ValueError`, so an + ``except ValueError`` caller is unaffected. :meta private: """ + if not isinstance(key, str): + # NOTE: unreachable only by the ``str`` annotation, which a caller + # at run time need not honour -- this is the path GitHub issue + # #1824 fixed. + return super().get(key) # type: ignore[unreachable] name = key.upper() - if name not in {NAME}._member_map_: # type: ignore[misc] # pylint: disable=no-member + if name not in cls._member_map_: # pylint: disable=no-member # NOTE: the value is ``default`` if the caller supplied one, or # else ``key`` exactly as given -- never ``name`` -- so an # unregistered member's value is the caller's own casing, the @@ -429,8 +468,8 @@ def get(key: 'str', default: 'Optional[str]' = None) -> '{NAME}': # ``_member_map_`` cache, the *first* casing seen for every later # call regardless of case; that is the one observable behaviour # change in GitHub issue #860's conversion. - return {NAME}._unregistered_member(default if default is not None else key, name) - return {NAME}[name] # type: ignore[misc] + return cls._unregistered_member(default if default is not None else key, name) + return cls._member_map_[name] # pylint: disable=no-member @classmethod def _validate_value(cls, value: 'Any') -> 'None': @@ -439,11 +478,13 @@ def _validate_value(cls, value: 'Any') -> 'None': Overrides :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` with the check :meth:`_missing_` makes, and the message it raises, so :meth:`register` refuses a value the constructor refuses, in the same - words (:issue:`1816`). Any :class:`str` passes, since - :meth:`_missing_` resolves every one. + words (:issue:`1816`). :meth:`get` reaches it as well, through the + base, for every non-:class:`str` key (:issue:`1824`), so it refuses + the same values. Any :class:`str` passes, since :meth:`_missing_` + resolves every one. Args: - value: Value about to be minted. + value: Value about to be looked up or minted. Raises: EnumValueError: If ``value`` is not a :class:`str`. A diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index e5f83f1d5..9033a057f 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -1169,8 +1169,9 @@ def test_the_issue_804_templates_render_the_committed_modules(self) -> None: # and both now close their enumeration block with a # ``_unregistered_member`` classmethod (the override each # needed for its own extra per-member attributes) rather - # than falling straight through to the ``get()`` - # staticmethod each still keeps. + # than falling straight through to the ``get()`` override + # each still keeps -- a classmethod on both since #908 and + # #1824. block = re.compile( rf'class {cls_name}\(EnumRegistry, StrEnum\):\n """.*?\n\n (#:.*?)' r'\n\n @classmethod', re.S) diff --git a/tests/const/test_const_ftp_command_get_key_type_1824_unit.py b/tests/const/test_const_ftp_command_get_key_type_1824_unit.py new file mode 100644 index 000000000..8869e0f4b --- /dev/null +++ b/tests/const/test_const_ftp_command_get_key_type_1824_unit.py @@ -0,0 +1,191 @@ +# -*- coding: utf-8 -*- +"""``Command.get`` refuses a non-``str`` key the way ``Method.get`` does. + +GitHub issue #1824. :meth:`~pcapkit.const.ftp.command.Command.get` was a +:class:`staticmethod` that called ``key.upper()`` on whatever it was given, so +``Command.get(b'\\x01')`` built an unregistered member keyed by the +:class:`bytes` object, and ``Command.get(-1)`` raised a builtin +:exc:`AttributeError` whether or not ``default`` was supplied. +:meth:`~pcapkit.const.http.method.Method.get` had the same shape until #908, +which made it a :class:`classmethod` delegating to +:meth:`~pcapkit.corekit.enum.EnumLookup.get`. + +``Command.get`` now follows it: a non-``str`` key goes to the base, whose call +to ``Command._validate_value`` (added by #1823) raises +:exc:`~pcapkit.utilities.exceptions.EnumValueError`. ``default`` is not +forwarded there, exactly as ``Method.get`` forwards none, because this +override's ``default`` is the *value* of the unregistered member it builds on a +name miss rather than the base's already-registered fallback -- so a refused key +raises whatever ``default`` is. + +A ``str`` key is untouched: case-insensitive per :rfc:`959#section-5`, with the +unregistered-member fallback keeping the caller's own casing as its value and +the upper-cased form as its name, as the ``NOTE`` in the override says. + +""" +import inspect +import unittest + + +class NonStrKeyTests(unittest.TestCase): + """A non-``str`` key is refused with the in-library error and mints nothing.""" + + def setUp(self) -> 'None': + from pcapkit.const.ftp.command import Command + + self.before = (len(Command._member_map_), len(Command._value2member_map_)) + + def tearDown(self) -> 'None': + from pcapkit.const.ftp.command import Command + + self.assertEqual((len(Command._member_map_), len(Command._value2member_map_)), + self.before, 'a non-str lookup must not grow either table') + + def _refused(self, key: 'object', **kwargs: 'object') -> 'BaseException': + from pcapkit.const.ftp.command import Command + from pcapkit.utilities.exceptions import EnumValueError + + with self.assertRaises(EnumValueError) as caught: + Command.get(key, **kwargs) # type: ignore[arg-type] + self.assertIs(type(caught.exception), EnumValueError) + self.assertEqual(str(caught.exception), f'{key!r} is not a valid Command') + return caught.exception + + def test_a_bytes_key_raises_instead_of_returning_a_bytes_keyed_member(self) -> 'None': + """Was ```` -- a member, not an error.""" + self._refused(b'\x01') + self._refused(b'ABOR') + + def test_an_int_key_raises_the_in_library_error(self) -> 'None': + """Was a builtin :exc:`AttributeError` from ``key.upper()``.""" + error = self._refused(-1) + self.assertNotIsInstance(error, AttributeError) + self.assertIsInstance(error, ValueError) + + def test_an_int_key_with_a_default_still_raises(self) -> 'None': + """``default`` names the unregistered member's value, so it cannot rescue a + refused key -- not even when it is a registered command's value.""" + self._refused(-1, default='XYZW') + self._refused(-1, default='ABOR') + + def test_other_non_str_keys_raise_too(self) -> 'None': + self._refused(None) + self._refused(1.5) + + def test_the_refusal_matches_method_get(self) -> 'None': + """Same exception class and message shape as ``Method.get``, with a default.""" + from pcapkit.const.http.method import Method + from pcapkit.utilities.exceptions import EnumValueError + + for key in (-1, b'\x01'): + with self.subTest(key=key): + with self.assertRaises(EnumValueError) as method: + Method.get(key, default='GET') # type: ignore[arg-type] + command = self._refused(key, default='ABOR') + self.assertIs(type(command), type(method.exception)) + self.assertEqual(str(command).replace('Command', 'Method'), + str(method.exception)) + + +class StrKeyTests(unittest.TestCase): + """A ``str`` key keeps the case-insensitive lookup and the fallback.""" + + def setUp(self) -> 'None': + from pcapkit.const.ftp.command import Command + + self.before = (len(Command._member_map_), len(Command._value2member_map_)) + + def tearDown(self) -> 'None': + from pcapkit.const.ftp.command import Command + + self.assertEqual((len(Command._member_map_), len(Command._value2member_map_)), + self.before, 'a str lookup must not grow either table') + + def test_a_registered_key_resolves(self) -> 'None': + from pcapkit.const.ftp.command import Command + + self.assertIs(Command.get('ABOR'), Command.ABOR) # type: ignore[attr-defined] + self.assertIs(Command.get('ABOR', 'XYZW'), Command.ABOR) # type: ignore[attr-defined] + + def test_a_registered_key_in_another_case_resolves(self) -> 'None': + from pcapkit.const.ftp.command import Command + + for key in ('abor', 'Abor', 'aBoR'): + with self.subTest(key=key): + self.assertIs(Command.get(key), Command.ABOR) # type: ignore[attr-defined] + self.assertIs(Command.get(key, 'XYZW'), Command.ABOR) # type: ignore[attr-defined] + + def test_an_unregistered_key_keeps_the_callers_case(self) -> 'None': + from pcapkit.const.ftp.command import Command + + lower, upper = Command.get('xyzw'), Command.get('XYZW') + self.assertEqual((lower.value, lower.name), ('xyzw', 'XYZW')) + self.assertEqual((upper.value, upper.name), ('XYZW', 'XYZW')) + self.assertNotEqual(lower, upper) + self.assertNotIn(lower, list(Command)) + + def test_an_unregistered_key_takes_its_value_from_default(self) -> 'None': + from pcapkit.const.ftp.command import Command + + built = Command.get('xyzw', 'Placeholder') + self.assertEqual((built.value, built.name), ('Placeholder', 'XYZW')) + + +class ShapeTests(unittest.TestCase): + """The override's shape, in the generated module and in its template.""" + + def test_get_is_a_classmethod_that_delegates(self) -> 'None': + from pcapkit.const.ftp.command import Command + + descriptor = inspect.getattr_static(Command, 'get') + self.assertIsInstance(descriptor, classmethod) + self.assertIn('return super().get(key)', inspect.getsource(descriptor.__func__)) + + def test_the_docstring_records_the_change(self) -> 'None': + from pcapkit.const.ftp.command import Command + + doc = inspect.getdoc(Command.get) or '' + self.assertIn('.. versionchanged:: 1.5.0', doc) + self.assertRegex(doc, r'Raises:\n\s+EnumValueError: If ``key`` is not a :class:`str`') + self.assertIn(':issue:`1824`', doc) + + def test_the_crawler_template_carries_the_same_get(self) -> 'None': + """The fix lives in :data:`pcapkit.vendor.ftp.command.LINE`, so a + regeneration cannot discard it. Rendering the template needs no network.""" + from pcapkit.const.ftp.command import Command + from pcapkit.vendor.ftp.command import LINE + + rendered = LINE('Command', 'FTP Command', '', '', 'pcapkit.vendor.ftp.command') + # NOTE: through ``getattr_static`` because both descriptor kinds carry + # ``__func__`` there, while ``Command.get.__func__`` exists only on the + # bound classmethod -- so this compares sources on either shape. + source = inspect.getsource(inspect.getattr_static(Command, 'get').__func__) + self.assertIn(source.rstrip('\n'), rendered) + # A count rather than ``assertIn``, so a failure reports a number instead + # of the whole rendered module. + self.assertEqual(rendered.count(' return super().get(key) '), 1, + "the template's Command.get does not delegate a non-str key") + + +class FEATCodeUnaffectedTests(unittest.TestCase): + """``FEATCode.get`` already delegated a non-``str`` key, so it never had the defect.""" + + def test_a_non_str_key_raises_the_in_library_error(self) -> 'None': + from pcapkit.const.ftp.command import FEATCode + from pcapkit.utilities.exceptions import EnumValueError + + for key in (b'\x01', -1, None): + with self.subTest(key=key): + with self.assertRaises(EnumValueError): + FEATCode.get(key) + + def test_its_default_is_the_bases_registered_fallback(self) -> 'None': + """Unlike ``Command.get``'s, ``FEATCode.get``'s ``default`` is the base's own, + so a refused key falls back to it, as on every registry that inherits it.""" + from pcapkit.const.ftp.command import FEATCode + + self.assertIs(FEATCode.get(-1, ''), FEATCode.base) # type: ignore[attr-defined] + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/const/test_const_register_validates_every_registry_1816_unit.py b/tests/const/test_const_register_validates_every_registry_1816_unit.py index 11bfa5f81..689da7467 100644 --- a/tests/const/test_const_register_validates_every_registry_1816_unit.py +++ b/tests/const/test_const_register_validates_every_registry_1816_unit.py @@ -22,10 +22,11 @@ The oracle is ``get`` with no default, except where ``get`` is a :class:`staticmethod` backport that bypasses the base lookup. ``OptionType``'s -answers every integer code with an unregistered member, and ``Command``'s calls -``key.upper()`` and so answers :class:`bytes` too. There the constructor is the -oracle instead; ``OptionType``'s ``_missing_`` refusing anything above -``0xFFFF`` is what the issue cites. +answers every integer code with an unregistered member, so there the constructor +is the oracle instead; ``OptionType``'s ``_missing_`` refusing anything above +``0xFFFF`` is what the issue cites. ``Command``'s was one too, calling +``key.upper()`` and so answering :class:`bytes`, until #1824 made it a +:class:`classmethod` that hands a non-``str`` key to the base. Each test imports the registries after :func:`~tests._support.reimport_once_per_class`, whose ``restore`` puts the class's diff --git a/tests/project/test_conventions_doc_claims.py b/tests/project/test_conventions_doc_claims.py index ded55a56e..b38d525af 100644 --- a/tests/project/test_conventions_doc_claims.py +++ b/tests/project/test_conventions_doc_claims.py @@ -807,12 +807,13 @@ def test_a_delegating_override_is_a_classmethod(self) -> 'None': first positional parameter, so in ``get(key, ...)`` it binds the lookup key and raises :exc:`TypeError` -- ``RuntimeError: super(): no arguments`` needs a function with no parameters at all, which no real override has. - #913 set the shape and #908 followed it, producing ``Method.get``. The two - surviving ``@staticmethod`` overrides are the stated exception -- neither - calls ``super()``, so neither meets the condition. Read through :func:`vars` - rather than by attribute access, since both descriptor kinds answer - ``Cls.get('X')`` identically, which is the page's own point about callers not - seeing the switch. + #913 set the shape and #908 followed it, producing ``Method.get``; #1824 + followed it again for ``Command.get``, which delegates a non-``str`` key. + ``OptionType.get``, the surviving ``@staticmethod`` override, is the stated + exception -- it never calls ``super()``, so it does not meet the condition. + Read through :func:`vars` rather than by attribute access, since both + descriptor kinds answer ``Cls.get('X')`` identically, which is the page's own + point about callers not seeing the switch. """ from pcapkit.const.ftp.command import Command @@ -838,17 +839,22 @@ def test_a_delegating_override_is_a_classmethod(self) -> 'None': 'override folded into the inherited base is what #940 did ' 'to the mh/ngap helpers') - self.assertIsInstance(vars(Method)['get'], classmethod, - 'a delegating override has to be a classmethod -- ' - 'zero-argument super() in a staticmethod binds the ' - 'first positional parameter, so get(key) raises ' - 'TypeError; #913 set the shape, #908 followed it') - for klass in (Command, OptionType): + for klass in (Method, Command): with self.subTest(klass=klass.__name__): - self.assertIsInstance(vars(klass)['get'], staticmethod) - self.assertNotIn('super()', inspect.getsource(vars(klass)['get'].__func__), - f'{klass.__name__}.get now delegates, so the page is ' - 'wrong to list it as a surviving staticmethod') + self.assertIsInstance(vars(klass)['get'], classmethod, + 'a delegating override has to be a classmethod -- ' + 'zero-argument super() in a staticmethod binds the ' + 'first positional parameter, so get(key) raises ' + 'TypeError; #913 set the shape, #908 and #1824 ' + 'followed it') + self.assertIn('return super().get(key)', + inspect.getsource(vars(klass)['get'].__func__), + f'{klass.__name__}.get no longer delegates, so the page is ' + 'wrong to give it as a delegating override') + self.assertIsInstance(vars(OptionType)['get'], staticmethod) + self.assertNotIn('super()', inspect.getsource(vars(OptionType)['get'].__func__), + 'OptionType.get now delegates, so the page is wrong to list it ' + 'as the surviving staticmethod') # Execute the three outcomes rather than assert the page names them. The # first draft of this test only grepped for the error string, and the @@ -919,6 +925,10 @@ def get(): # type: ignore[override] # noqa: D102 self.assertIn('forced by Python rather than decided', flat, 'the page no longer says the classmethod requirement is a ' 'language constraint rather than a ruling') + self.assertIn("Command`'s override made the same switch on :issue:`1824`", flat, + 'the page no longer records that Command.get became a ' + 'classmethod once it delegated, so it reads as a surviving ' + 'staticmethod again') def test_the_base_raises_a_name_miss_quietly(self) -> 'None': """#933: the owner ruled that overrides follow the base and stay quiet.