From 75e485b313d1467e56d239841f562cbafe8617c5 Mon Sep 17 00:00:00 2001 From: "jsh9, PhD" <25124332+jsh9@users.noreply.github.com> Date: Tue, 16 Jan 2024 01:17:53 -0500 Subject: [PATCH] Fix false positive DOC203 in property methods (#115) --- CHANGELOG.md | 10 ++++ pydoclint/utils/generic.py | 24 ---------- pydoclint/utils/special_methods.py | 32 +++++++++++++ pydoclint/visitor.py | 15 ++++-- setup.cfg | 2 +- tests/data/common/property_method.py | 12 +++++ tests/test_main.py | 10 ++++ tests/utils/test_generic.py | 57 +--------------------- tests/utils/test_special_methods.py | 72 ++++++++++++++++++++++++++++ 9 files changed, 151 insertions(+), 83 deletions(-) create mode 100644 pydoclint/utils/special_methods.py create mode 100644 tests/data/common/property_method.py create mode 100644 tests/utils/test_special_methods.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 115a9dc..4c782a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Change Log +## [0.3.9] - 2024-01-16 + +- Fixed + + - False positive violation `DOC203` when there is no docstring return section + for methods with `@property` decorator + +- Full diff + - https://github.com/jsh9/pydoclint/compare/0.3.8...0.3.9 + ## [0.3.8] - 2023-10-20 - Fixed diff --git a/pydoclint/utils/generic.py b/pydoclint/utils/generic.py index 75c4e27..afb1537 100644 --- a/pydoclint/utils/generic.py +++ b/pydoclint/utils/generic.py @@ -92,19 +92,6 @@ def detectMethodType(node: ast.FunctionDef) -> MethodType: return MethodType.INSTANCE_METHOD -def checkIsAbstractMethod(node: ast.FunctionDef) -> bool: - """Check whether `node` is an abstract method""" - if len(node.decorator_list) == 0: - return False - - for decorator in node.decorator_list: - if isinstance(decorator, ast.Name): - if decorator.id == 'abstractmethod': - return True - - return False - - def getDocstring(node: ClassOrFunctionDef) -> str: """Get docstring from a class definition or a function definition""" docstring_: Optional[str] = ast.get_docstring(node) @@ -153,17 +140,6 @@ def getNodeName(node: ast.AST) -> str: return node.name if 'name' in node.__dict__ else '' -def isPropertyMethod(node: FuncOrAsyncFuncDef) -> bool: - """Check whether a function has `@property` as its last decorator""" - return ( - isinstance(node.decorator_list, list) - and len(node.decorator_list) > 0 - and isinstance(node.decorator_list[-1], ast.Name) - and hasattr(node.decorator_list[-1], 'id') - and node.decorator_list[-1].id == 'property' - ) - - def stringStartsWith(string: str, substrings: Tuple[str, ...]) -> bool: """Check whether the string starts with any of the substrings""" for substring in substrings: diff --git a/pydoclint/utils/special_methods.py b/pydoclint/utils/special_methods.py new file mode 100644 index 0000000..017d480 --- /dev/null +++ b/pydoclint/utils/special_methods.py @@ -0,0 +1,32 @@ +import ast + +from pydoclint.utils.astTypes import FuncOrAsyncFuncDef + + +def checkIsAbstractMethod(node: FuncOrAsyncFuncDef) -> bool: + """Check whether `node` is an abstract method""" + return checkMethodContainsSpecifiedDecorator(node, 'abstractmethod') + + +def checkIsPropertyMethod(node: FuncOrAsyncFuncDef) -> bool: + """Check whether `node` is a method with @property decorator""" + return checkMethodContainsSpecifiedDecorator(node, 'property') + + +def checkMethodContainsSpecifiedDecorator( + node: FuncOrAsyncFuncDef, + decorator: str, +) -> bool: + """Check whether a method is decorated by the specified decorator""" + return ( + isinstance(node.decorator_list, list) + and len(node.decorator_list) > 0 + and any( + ( # noqa: PAR001 + isinstance(_, ast.Name) + and hasattr(node.decorator_list[-1], 'id') + and _.id == decorator + ) + for _ in node.decorator_list + ) + ) diff --git a/pydoclint/visitor.py b/pydoclint/visitor.py index f2ebcdc..8bf1939 100644 --- a/pydoclint/visitor.py +++ b/pydoclint/visitor.py @@ -6,12 +6,10 @@ from pydoclint.utils.astTypes import FuncOrAsyncFuncDef from pydoclint.utils.doc import Doc from pydoclint.utils.generic import ( - checkIsAbstractMethod, collectFuncArgs, detectMethodType, generateMsgPrefix, getDocstring, - isPropertyMethod, ) from pydoclint.utils.internal_error import InternalError from pydoclint.utils.method_type import MethodType @@ -27,6 +25,10 @@ isReturnAnnotationNone, isReturnAnnotationNoReturn, ) +from pydoclint.utils.special_methods import ( + checkIsAbstractMethod, + checkIsPropertyMethod, +) from pydoclint.utils.violation import Violation from pydoclint.utils.visitor_helper import ( checkReturnTypesForViolations, @@ -485,11 +487,12 @@ def checkReturns( # noqa: C901 hasGenAsRetAnno: bool = hasGeneratorAsReturnAnnotation(node) onlyHasYieldStmt: bool = hasYieldStmt and not hasReturnStmt hasIterAsRetAnno: bool = hasIteratorOrIterableAsReturnAnnotation(node) + isPropertyMethod: bool = checkIsPropertyMethod(node) docstringHasReturnSection: bool = doc.hasReturnsSection violations: List[Violation] = [] - if not docstringHasReturnSection and not isPropertyMethod(node): + if not docstringHasReturnSection and not isPropertyMethod: if ( # fmt: off not (onlyHasYieldStmt and hasIterAsRetAnno) @@ -541,6 +544,12 @@ def checkReturns( # noqa: C901 # to check for DOC203 violations. return violations + if returnSec == [] and isPropertyMethod: + # No need to check return type for methods with "@property" + # decorator. This is because it's OK for @property methods + # to have no return section in the docstring. + return violations + checkReturnTypesForViolations( style=self.style, returnAnnotation=returnAnno, diff --git a/setup.cfg b/setup.cfg index 6e4dcd2..aef804a 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,6 @@ [metadata] name = pydoclint -version = 0.3.8 +version = 0.3.9 description = A Python docstring linter that checks arguments, returns, yields, and raises sections long_description = file: README.md long_description_content_type = text/markdown diff --git a/tests/data/common/property_method.py b/tests/data/common/property_method.py new file mode 100644 index 0000000..442048a --- /dev/null +++ b/tests/data/common/property_method.py @@ -0,0 +1,12 @@ +class MyClass: + data: float = 2.1 + + @property + def something(self) -> float: + """ + Some property. + + It's OK to have no return section in this method, because this + is a "property method" and is intended to be used as an attribute. + """ + return self.data diff --git a/tests/test_main.py b/tests/test_main.py index c796612..e4245c7 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -900,6 +900,16 @@ def testAbstractMethod(style: str, checkReturnTypes: bool) -> None: assert list(map(str, violations)) == expected +@pytest.mark.parametrize('style', ['google', 'numpy', 'sphinx']) +def testNoReturnSectionInPropertyMethod(style: str) -> None: + violations = _checkFile( + filename=DATA_DIR / 'common/property_method.py', + style=style, + skipCheckingShortDocstrings=False, + ) + assert len(violations) == 0 + + @pytest.mark.parametrize( 'style, argTypeHintsInDocstring, argTypeHintsInSignature', itertools.product( diff --git a/tests/utils/test_generic.py b/tests/utils/test_generic.py index 3bcab14..c3b24fa 100644 --- a/tests/utils/test_generic.py +++ b/tests/utils/test_generic.py @@ -1,13 +1,9 @@ import ast -from typing import List, Optional +from typing import List import pytest -from pydoclint.utils.generic import ( - collectFuncArgs, - isPropertyMethod, - stripQuotes, -) +from pydoclint.utils.generic import collectFuncArgs, stripQuotes src1 = """ def func1( @@ -76,55 +72,6 @@ def testCollectFuncArgs(src: str, expected: List[str]) -> None: assert [_.arg for _ in out] == expected -srcProperty1 = """ -class A: - def method1(self): - pass -""" - -srcProperty2 = """ -class A: - @property - def method1(self): - pass -""" - -srcProperty3 = """ -# pydoclint only does static code analysis in order to achieve fast speed. -# If users rename built-in decorator names (such as `property`), pydoclint -# will not recognize it. - -hello_world = property - -class A: - @hello_world - def method1(self): - pass -""" - - -@pytest.mark.parametrize( - 'src, expected', - [ - (srcProperty1, False), - (srcProperty2, True), - (srcProperty3, False), - ], -) -def testIsPropertyMethod(src: str, expected: bool) -> None: - def getMethod1(tree_: ast.AST) -> Optional[ast.FunctionDef]: - for node_ in ast.walk(tree_): - if isinstance(node_, ast.FunctionDef) and node_.name == 'method1': - return node_ - - return None - - tree = ast.parse(src) - node = getMethod1(tree) - result = isPropertyMethod(node) - assert result == expected - - @pytest.mark.parametrize( 'string, expected', [ diff --git a/tests/utils/test_special_methods.py b/tests/utils/test_special_methods.py new file mode 100644 index 0000000..6d8b188 --- /dev/null +++ b/tests/utils/test_special_methods.py @@ -0,0 +1,72 @@ +import ast +from typing import Optional + +import pytest + +from pydoclint.utils.special_methods import ( + checkMethodContainsSpecifiedDecorator, +) + +src1 = """ +class A: + def method1(self): + pass +""" + +src2 = """ +class A: + @property + def method1(self): + pass +""" + +src3 = """ +class A: + @hello + @world + @property + @morning + def method1(self): + pass +""" + +src4 = """ +# pydoclint only does static code analysis in order to achieve fast speed. +# If users rename built-in decorator names (such as `property`), pydoclint +# will not recognize it. + +hello_world = property + +class A: + @hello_world + def method1(self): + pass +""" + + +@pytest.mark.parametrize( + 'src, decorator, expected', + [ + (src1, 'something', False), + (src2, 'property', True), + (src3, 'property', True), + (src4, 'hello_world', True), + (src4, 'property', False), + ], +) +def testCheckMethodContainsSpecifiedDecorator( + src: str, + decorator: str, + expected: bool, +) -> None: + def getMethod1(tree_: ast.AST) -> Optional[ast.FunctionDef]: + for node_ in ast.walk(tree_): + if isinstance(node_, ast.FunctionDef) and node_.name == 'method1': + return node_ + + return None + + tree = ast.parse(src) + node = getMethod1(tree) + result = checkMethodContainsSpecifiedDecorator(node, decorator=decorator) + assert result == expected