deepyaman commented on code in PR #29061: URL: https://github.com/apache/flink/pull/29061#discussion_r3920947997
########## 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: Is this really the best approach? How is this _usually_ done? ########## 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: Any reason not to use a triple-quoted string instead? ########## 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: What's the purpose of the `try`/`except` here? ########## 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: All the changes (and also the existing code) is very complicated! Any reason this can't use `@deprecated` from `typing-extensions>=4.5.0`, at least as a base? If not, could a lot not at least be learned from the PEP-702 implementation? ########## 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: Why not use something like `assertWarns` (or `pytest.raises`, although PyFlink doesn't seem to generally use `pytest` for whatever 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: Again, is this actually being exercised? Or is this unnecessarily-defensive coding? -- 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]
