deepyaman commented on code in PR #29061: URL: https://github.com/apache/flink/pull/29061#discussion_r3926236676
########## 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: Still, instead of doing this, why can't we just us `pytest.warns`? Would there be some incompatibility with unittest? I assume not... ########## 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: Are these `cast`s wholly necessary? If it's necessary for type checking, I get it, but just want to be sure. ########## flink-python/pyflink/util/tests/test_api_stability_decorators.py: ########## Review Comment: What's with all these `Cls` inheriting from `object`? Isn't that more of a Python 2 construct? ########## 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: Is this level of detail re specific examples necessary for a docstring? -- 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]
