deepyaman commented on code in PR #29061:
URL: https://github.com/apache/flink/pull/29061#discussion_r3927933603
##########
flink-python/pyflink/util/api_stability_decorators.py:
##########
@@ -126,20 +136,95 @@ def __init__(self, since: str, detail: Optional[str] =
None):
self.detail = detail
def get_directive(self, func_or_cls: T) -> str:
- return f".. deprecated:: {self.since}\n{indent(dedent(self.detail), '
')}"
+ directive = f".. deprecated:: {self.since}"
+ if self.detail is not None:
+ directive = f"{directive}\n{indent(dedent(self.detail), ' ')}"
+ return directive
- @override
- def __call__(self, func_or_cls: T) -> T:
+ def _get_message(self, func_or_cls: T) -> str:
"""
- Emit a warning on the deprecation of the given function/class. Then
call the base class
- for docstring modification.
+ Returns the warning message emitted when the deprecated API element is
used.
"""
- msg = f"{func_or_cls.__qualname__} has been deprecated since version
{self.since}."
+ name = getattr(func_or_cls, "__qualname__", None) or getattr(
+ func_or_cls, "__name__", "This API"
+ )
+ msg = f"{name} has been deprecated since version {self.since}."
if self.detail is not None:
msg = f"{msg} {self.detail}"
+ return msg
+
+ @override
+ def __call__(self, func_or_cls: T) -> T:
Review Comment:
Adopted for functions in aa4e7e6c — that path is now just
`deprecated(self._get_message(func))(func)`, and the hand-written wrapper is
gone (−67/+19 in this file).
Not for classes, though, and not as the base for everything. PEP 702 warns
when a deprecated class is **subclassed** as well as when it is instantiated,
and `Rowtime`/`Schema` extend the deprecated `Descriptor` at module level, so
adopting it wholesale reintroduces exactly the bug this PR fixes:
```
import pyflink.table.descriptors # with typing_extensions.deprecated
→ DeprecationWarning @ <frozen abc>:106 -> Descriptor has been deprecated
since version 2.1.0.
→ DeprecationWarning @ <frozen abc>:106 -> Descriptor has been deprecated
since version 2.1.0.
```
with worse attribution than before, since `<frozen abc>` is where `ABCMeta`
creates the class.
Two more findings from the same investigation:
* it raises `TypeError` on a `classmethod` or a `property` on every
supported Python version;
* on 3.10+ it *accepts* a `staticmethod` and returns a plain function, so
`H.sm(1)` works but `H().sm(1)` then fails with `takes 1 positional argument
but 2 were given`.
That's what the descriptor re-packaging is for. Both cases now have
regression tests, as does the subclass-definition one.
The `object.__new__` excess-argument check in the class path is borrowed
from PEP 702's implementation, with a comment saying so.
One thing this surfaced: `typing_extensions` has been imported by this
module since FLINK-37365 without ever being declared as a dependency — it
resolved only because `apache-beam` happens to require it. It's now in
`install_requires` with the `>=4.5.0` floor that `typing_extensions.deprecated`
needs.
##########
flink-python/pyflink/util/tests/test_api_stability_decorators.py:
##########
@@ -33,13 +35,17 @@
)
[email protected]
def _catch_warnings():
"""
- Returns a context manager recording every warning raised within it.
+ Records every warning raised within the block.
+
+ Used where :func:`unittest.TestCase.assertWarns` cannot express the
assertion: that
+ nothing warned, or that something warned exactly once.
"""
- context = warnings.catch_warnings(record=True)
- warnings.simplefilter("always")
- return context
+ with warnings.catch_warnings(record=True) as caught:
+ warnings.simplefilter("always")
+ yield caught
Review Comment:
No incompatibility with unittest — `pytest.warns` works fine inside a
`TestCase`. Two other things, though:
* `pytest.warns(None)`, the "assert nothing warned" form, was deprecated in
pytest 7 and removed in 8, and we pin `pytest~=8.0`: it now raises `TypeError:
exceptions must be derived from Warning, not <class 'NoneType'>`. Nine of these
tests assert absence, so they'd need `catch_warnings` regardless and
`pytest.warns` would be a second idiom beside it.
* Exactly one module in all of `pyflink` touches pytest, via a
function-local `import pytest` inside a single test method. A module-level
pytest import here would be a first.
Moot now anyway: 65ca902 deletes the helper. The absence assertions run
under `simplefilter("error")`, so a stray warning fails at the line that raised
it and names the API, instead of surfacing as `[] != [...]` afterwards; the
three that count warnings or compare messages use `catch_warnings(record=True)`
directly. The module is 13 lines shorter without the helper than with it.
##########
flink-python/pyflink/util/tests/test_api_stability_decorators.py:
##########
Review Comment:
Agreed — dropped throughout the test module in 2bf5f6a.
For what it's worth the codebase leans the other way (229 `class X(object)`
against 13 bare), but that's inherited Python 2 style in old production code;
no reason for new tests to copy it.
##########
flink-python/pyflink/util/api_stability_decorators.py:
##########
@@ -174,57 +170,50 @@ def __call__(self, func_or_cls: T) -> T:
if isclass(func_or_cls):
self._deprecate_class(func_or_cls)
elif isfunction(func_or_cls):
- return cast(T, self._deprecate_function(func_or_cls))
- # Anything else (a property, for instance) cannot be wrapped without
changing what the
- # decorated name refers to, so the docstring directive is all we apply.
+ # PEP 702's implementation, by way of its typing_extensions
backport: a
+ # functools.wraps wrapper that warns with the caller's stacklevel,
plus the
+ # __deprecated__ attribute that type checkers read.
+ return cast(T,
deprecated(self._get_message(func_or_cls))(func_or_cls))
Review Comment:
Necessary for type checking. mypy runs over this file in CI — it isn't in
`tox.ini`'s `files=` list, but it's pulled in through `pyflink/table/*.py`.
Removing both casts:
```
api_stability_decorators.py:166: error: Incompatible return value type (got
"staticmethod[Any, Any]", expected "T")
api_stability_decorators.py:176: error: Incompatible return value type (got
"FunctionType", expected "T")
```
`T` is bound to `Union[Callable[..., Any], Type[Any]]`, and neither the
re-packaged descriptor nor the return of `typing_extensions.deprecated` narrows
back to the *same* `T` the caller passed in.
Leaving open in case you'd rather restructure the signature so they aren't
needed.
##########
flink-python/pyflink/util/api_stability_decorators.py:
##########
@@ -174,57 +170,50 @@ def __call__(self, func_or_cls: T) -> T:
if isclass(func_or_cls):
self._deprecate_class(func_or_cls)
elif isfunction(func_or_cls):
- return cast(T, self._deprecate_function(func_or_cls))
- # Anything else (a property, for instance) cannot be wrapped without
changing what the
- # decorated name refers to, so the docstring directive is all we apply.
+ # PEP 702's implementation, by way of its typing_extensions
backport: a
+ # functools.wraps wrapper that warns with the caller's stacklevel,
plus the
+ # __deprecated__ attribute that type checkers read.
+ return cast(T,
deprecated(self._get_message(func_or_cls))(func_or_cls))
+
+ # A property is neither, and typing_extensions.deprecated rejects it.
Replacing the
+ # descriptor to warn on attribute access is not worth it for a
deprecated API, so
+ # the docstring directive is all we apply.
return func_or_cls
- def _deprecate_function(self, func: Callable[..., Any]) -> Callable[...,
Any]:
- """
- Returns a wrapper around the given function that warns before
delegating to it.
- """
- msg = self._get_message(func)
-
- @functools.wraps(func)
- def wrapper(*args: Any, **kwargs: Any) -> Any:
- # stacklevel=2 attributes the warning to the caller of the
deprecated function
- # rather than to this wrapper.
- warnings.warn(msg, category=DeprecationWarning, stacklevel=2)
- return func(*args, **kwargs)
-
- return wrapper
-
def _deprecate_class(self, cls: Type[Any]) -> None:
"""
Wraps the __init__ of the given class so that instantiating it warns.
- The class itself is returned unchanged by :func:`__call__`; replacing
it with a wrapper
- would break isinstance checks and subclassing.
+ typing_extensions.deprecated is deliberately not used for classes.
Following PEP 702
+ it warns when a deprecated class is *subclassed* as well as when it is
instantiated,
+ and PyFlink subclasses its own deprecated classes -- Rowtime and
Schema in
+ pyflink.table.descriptors both extend the deprecated Descriptor -- so
that warning
Review Comment:
No — trimmed in 2bf5f6a. The docstring now just says PyFlink subclasses its
own deprecated classes at module level, without naming
`Rowtime`/`Schema`/`Descriptor`; those names would rot as
`pyflink.table.descriptors` changes, and the full argument lives in the commit
message and the PR description.
2bf5f6a also does a wider pass over both files, cutting comments that
restated the code and keeping the ones that explain why: why the warning can't
be emitted at decoration time, why `typing_extensions.deprecated` covers
functions but not classes, why a property is documented-only, why the
subprocess, and why `object.__new__`'s check is re-raised by hand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
