deepyaman commented on code in PR #29061: URL: https://github.com/apache/flink/pull/29061#discussion_r3927929101
########## flink-python/pyflink/util/tests/test_api_stability_decorators.py: ########## @@ -0,0 +1,410 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ +import abc +import enum +import inspect +import os +import subprocess +import sys +import unittest +import warnings + +from pyflink.util.api_stability_decorators import ( + Deprecated, + Experimental, + Internal, + Public, + PublicEvolving, +) + + +def _catch_warnings(): + """ + Returns a context manager recording every warning raised within it. + """ + context = warnings.catch_warnings(record=True) + warnings.simplefilter("always") + return context + + Review Comment: Done in aa4e7e6c. Assertions that something warns now use `assertWarns`, and its context object also exposes `.filename`/`.lineno`, which simplified the two attribution tests as well. The helper it left behind for the absence assertions is gone entirely in 65ca902 — see the `pytest.warns` thread. ########## flink-python/pyflink/util/tests/test_api_stability_decorators.py: ########## @@ -0,0 +1,410 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ +import abc +import enum +import inspect +import os +import subprocess +import sys +import unittest +import warnings + +from pyflink.util.api_stability_decorators import ( + Deprecated, + Experimental, + Internal, + Public, + PublicEvolving, +) + + +def _catch_warnings(): + """ + Returns a context manager recording every warning raised within it. + """ + context = warnings.catch_warnings(record=True) + warnings.simplefilter("always") + return context + + +class DeprecatedTests(unittest.TestCase): + """ + Tests for the :class:`Deprecated` decorator, which must warn when a deprecated API is + used, and not when it is defined. + """ + + def test_decoration_does_not_warn(self): + with _catch_warnings() as caught: + + @Deprecated(since="1.0.0", detail="Use :func:`new_func` instead.") + def func(): + pass + + @Deprecated(since="1.0.0") + class Cls(object): + def __init__(self): + pass + + self.assertEqual([], [str(warning.message) for warning in caught]) + + def test_importing_pyflink_table_does_not_warn(self): + # A regression test for the decorators warning at import time: every deprecated API of + # pyflink.table used to warn as soon as the package was imported. This needs a fresh + # interpreter, as pyflink.table is already imported in the one running the tests. Review Comment: Keeping the subprocess, but the comment now says why. `pyflink.table` is already in `sys.modules` by the time this test runs, so importing it again is a no-op — a fresh interpreter is the only way to observe an import. CPython's own test suite does the same thing (`test.support.script_helper.assert_python_ok`). The script also inspects only warnings this decorator raises, so an unrelated `DeprecationWarning` from a third-party package can't fail it. It now imports `pyflink.table.descriptors` too, which is the module that subclasses a deprecated class at import time. Leaving this open in case you'd still rather it were done differently. ########## flink-python/pyflink/util/tests/test_api_stability_decorators.py: ########## @@ -0,0 +1,410 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ +import abc +import enum +import inspect +import os +import subprocess +import sys +import unittest +import warnings + +from pyflink.util.api_stability_decorators import ( + Deprecated, + Experimental, + Internal, + Public, + PublicEvolving, +) + + +def _catch_warnings(): + """ + Returns a context manager recording every warning raised within it. + """ + context = warnings.catch_warnings(record=True) + warnings.simplefilter("always") + return context + + +class DeprecatedTests(unittest.TestCase): + """ + Tests for the :class:`Deprecated` decorator, which must warn when a deprecated API is + used, and not when it is defined. + """ + + def test_decoration_does_not_warn(self): + with _catch_warnings() as caught: + + @Deprecated(since="1.0.0", detail="Use :func:`new_func` instead.") + def func(): + pass + + @Deprecated(since="1.0.0") + class Cls(object): + def __init__(self): + pass + + self.assertEqual([], [str(warning.message) for warning in caught]) + + def test_importing_pyflink_table_does_not_warn(self): + # A regression test for the decorators warning at import time: every deprecated API of + # pyflink.table used to warn as soon as the package was imported. This needs a fresh + # interpreter, as pyflink.table is already imported in the one running the tests. + script = ( + "import warnings\n" + "with warnings.catch_warnings(record=True) as caught:\n" + " warnings.simplefilter('always')\n" + " import pyflink.table\n" + "print([str(warning.message) for warning in caught\n" + " if 'has been deprecated since version' in str(warning.message)])\n" Review Comment: No reason — done in aa4e7e6c, `textwrap.dedent` around a triple-quoted block. ########## flink-python/pyflink/util/api_stability_decorators.py: ########## @@ -76,15 +77,24 @@ def __call__(self, func_or_cls: T) -> T: # Avoid duplicating directives if already present in the docstring. if directive not in docstring: - func_or_cls.__doc__ = f"{docstring}\n{directive}" + try: + func_or_cls.__doc__ = f"{docstring}\n{directive}" + except (AttributeError, TypeError): + pass Review Comment: No purpose — you're right, and it's removed in aa4e7e6c. Removing it fails no test, and every kind of object that actually gets decorated (function, class, property, `staticmethod`, `classmethod`) accepts `__doc__` assignment. Only extension types would reject it, and none are decorated. I removed the equivalent guard around replacing `__init__` for the same reason. ########## flink-python/pyflink/util/api_stability_decorators.py: ########## @@ -76,15 +77,24 @@ def __call__(self, func_or_cls: T) -> T: # Avoid duplicating directives if already present in the docstring. if directive not in docstring: - func_or_cls.__doc__ = f"{docstring}\n{directive}" + try: + func_or_cls.__doc__ = f"{docstring}\n{directive}" + except (AttributeError, TypeError): + pass # Add the decorator to an internal __stability_decorators set on the class/function # being decorated, for later introspection. if hasattr(func_or_cls, '__stability_decorators'): stability_decorators = getattr(func_or_cls, '__stability_decorators') stability_decorators.add(self.__class__) else: - setattr(func_or_cls, '__stability_decorators', {self.__class__}) + # Not every decorated object accepts attribute assignment (a property, for + # example). Those simply cannot be introspected; that is not a reason to fail + # at import time. + try: + setattr(func_or_cls, '__stability_decorators', {self.__class__}) + except (AttributeError, TypeError): + pass Review Comment: This one is exercised. Removing it: ``` FAILED ...::test_property — AttributeError: 'property' object has no attribute '__stability_decorators' ``` A `property` rejects arbitrary attribute assignment, so `@Deprecated` applied over a `@property` raises at import without the guard. The comment now names that case instead of hand-waving about "not every object". Leaving open in case you'd rather it were explicit about properties instead of a `try`/`except`. -- 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]
