From c4b1ffc5e0bf0429988253b05e833a54131a34fe Mon Sep 17 00:00:00 2001 From: jsh9 <25124332+jsh9@users.noreply.github.com> Date: Mon, 15 Jul 2024 01:42:47 -0700 Subject: [PATCH] Improve class attr comparison logic --- pydoclint/utils/arg.py | 58 ++----- pydoclint/utils/visitor_helper.py | 105 +++++------ .../google.py | 5 + .../13_class_attr_assignments/google.py | 26 +++ tests/test_main.py | 52 +++++- tests/utils/test_visitor_helper.py | 163 ++++++++++++++++++ 6 files changed, 303 insertions(+), 106 deletions(-) create mode 100644 tests/data/edge_cases/13_class_attr_assignments/google.py diff --git a/pydoclint/utils/arg.py b/pydoclint/utils/arg.py index 2427162..a756a90 100644 --- a/pydoclint/utils/arg.py +++ b/pydoclint/utils/arg.py @@ -88,25 +88,6 @@ def fromAstArg(cls, astArg: ast.arg) -> 'Arg': typeHint: str = '' if anno is None else unparseAnnotation(anno) return Arg(name=astArg.arg, typeHint=typeHint) - @classmethod - def fromAstAssignWithNonTupleTarget(cls, astAssign: ast.Assign) -> 'Arg': - """ - Construct an Arg object from a Python ast.Assign object whose - "target" field is an ast.Name rather than an ast.Tuple. - """ - if len(astAssign.targets) != 1: - raise InternalError( - f'astAssign.targets has length {len(astAssign.targets)}' - ) - - if not isinstance(astAssign.targets[0], ast.Name): # not a tuple - raise InternalError( - f'astAssign.targets[0] is of type {type(astAssign.targets[0])}' - ' instead of ast.Name' - ) - - return Arg(name=astAssign.targets[0].id, typeHint='') - @classmethod def fromAstAnnAssign(cls, astAnnAssign: ast.AnnAssign) -> 'Arg': """Construct an Arg object from a Python ast.AnnAssign object""" @@ -224,32 +205,25 @@ def fromDocstringAttr( return ArgList(infoList=infoList) @classmethod - def fromAstAssignWithTupleTarget(cls, astAssign: ast.Assign) -> 'ArgList': - """ - Construct an ArgList from a Python ast.Assign object whose - "target" field is an ast.Tuple rather than an ast.Name. - """ - if len(astAssign.targets) != 1: - raise InternalError( - f'astAssign.targets has length {len(astAssign.targets)}' - ) - - if not isinstance(astAssign.targets[0], ast.Tuple): - raise InternalError( - f'astAssign.targets[0] is of type {type(astAssign.targets[0])}' - ' instead of ast.Tuple' - ) - - infoList = [] - for i, item in enumerate(astAssign.targets[0].elts): - if not isinstance(item, ast.Name): + def fromAstAssign(cls, astAssign: ast.Assign) -> 'ArgList': + infoList: List[Arg] = [] + for i, target in enumerate(astAssign.targets): + if isinstance(target, ast.Tuple): # such as `a, b = c, d = 1, 2` + for j, item in enumerate(target.elts): + if not isinstance(item, ast.Name): + raise InternalError( + f'astAssign.targets[{i}].elts[{j}] is of' + f' type {type(item)} instead of ast.Name' + ) + + infoList.append(Arg(name=item.id, typeHint='')) + elif isinstance(target, ast.Name): # such as `a = 1` or `a = b = 2` + infoList.append(Arg(name=target.id, typeHint='')) + else: raise InternalError( - f'astAssign.targets[0].elts[{i}] is of type {type(item)}' - ' instead of ast.Name' + f'astAssign.targets[{i}] is of type {type(target)}' ) - infoList.append(Arg(name=item.id, typeHint='')) - return ArgList(infoList=infoList) def contains(self, arg: Arg) -> bool: diff --git a/pydoclint/utils/visitor_helper.py b/pydoclint/utils/visitor_helper.py index 8e61dda..8b11de0 100644 --- a/pydoclint/utils/visitor_helper.py +++ b/pydoclint/utils/visitor_helper.py @@ -1,11 +1,10 @@ """Helper functions to classes/methods in visitor.py""" import ast import sys -from typing import List, Optional, Set, Union +from typing import List, Optional, Set from pydoclint.utils.annotation import unparseAnnotation from pydoclint.utils.arg import Arg, ArgList -from pydoclint.utils.astTypes import FuncOrAsyncFuncDef from pydoclint.utils.doc import Doc from pydoclint.utils.generic import ( appendArgsToCheckToV105, @@ -42,14 +41,12 @@ def checkClassAttributesAgainstClassDocstring( treatPropertyMethodsAsClassAttributes: bool, ) -> None: """Check class attribute list against the attribute list in docstring""" - classAttributes = _collectClassAttributes( + + actualArgs: ArgList = extractClassAttributesFromNode( node=node, shouldDocumentPrivateClassAttributes=( shouldDocumentPrivateClassAttributes ), - ) - actualArgs: ArgList = _convertClassAttributesIntoArgList( - classAttrs=classAttributes, treatPropertyMethodsAsClassAttrs=treatPropertyMethodsAsClassAttributes, ) @@ -124,71 +121,57 @@ def checkClassAttributesAgainstClassDocstring( ) -def _collectClassAttributes( +def extractClassAttributesFromNode( *, node: ast.ClassDef, shouldDocumentPrivateClassAttributes: bool, -) -> List[Union[ast.Assign, ast.AnnAssign, FuncOrAsyncFuncDef]]: - if 'body' not in node.__dict__ or len(node.body) == 0: - return [] - - attributes: List[Union[ast.Assign, ast.AnnAssign]] = [] - for item in node.body: - # Notes: - # - ast.Assign are something like "attr1 = 1.5" - # - ast.AnnAssign are something like "attr2: float = 1.5" - if isinstance(item, (ast.Assign, ast.AnnAssign)): - classAttrName: str = _getClassAttrName(item) - if shouldDocumentPrivateClassAttributes: - attributes.append(item) - else: - if not classAttrName.startswith('_'): - attributes.append(item) - - if isinstance( - item, (ast.AsyncFunctionDef, ast.FunctionDef) - ) and checkIsPropertyMethod(item): - attributes.append(item) - - return attributes - - -def _getClassAttrName(attrItem: Union[ast.Assign, ast.AnnAssign]) -> str: - if isinstance(attrItem, ast.Assign): - return attrItem.targets[0].id - - if isinstance(attrItem, ast.AnnAssign): - return attrItem.target.id - - raise InternalError(f'Unrecognized attrItem type: {type(attrItem)}') - - -def _convertClassAttributesIntoArgList( - *, - classAttrs: List[Union[ast.Assign, ast.AnnAssign, FuncOrAsyncFuncDef]], treatPropertyMethodsAsClassAttrs: bool, ) -> ArgList: + """ + Extract class attributes from an AST node. + + Parameters + ---------- + node : ast.ClassDef + The class definition + shouldDocumentPrivateClassAttributes : bool + Whether we should document private class attributes. If ``True``, + private class attributes will be included in the return value. + treatPropertyMethodsAsClassAttrs : bool + Whether we'd like to treat property methods as class attributes. + If ``True``, property methods will be included in the return value. + + Returns + ------- + ArgList + The argument list + """ + if 'body' not in node.__dict__ or len(node.body) == 0: + return ArgList([]) + atl: List[Arg] = [] - for attr in classAttrs: - if isinstance(attr, ast.AnnAssign): - atl.append(Arg.fromAstAnnAssign(attr)) - elif isinstance(attr, ast.Assign): - if isinstance(attr.targets[0], ast.Tuple): - atl.extend(ArgList.fromAstAssignWithTupleTarget(attr).infoList) - else: - atl.append(Arg.fromAstAssignWithNonTupleTarget(attr)) - elif isinstance(attr, (ast.AsyncFunctionDef, ast.FunctionDef)): - if treatPropertyMethodsAsClassAttrs: + for itm in node.body: + if isinstance(itm, ast.AnnAssign): # with type hints ("a: int = 1") + atl.append(Arg.fromAstAnnAssign(itm)) + elif isinstance(itm, ast.Assign): # no type hints + if not isinstance(itm.targets, list) or len(itm.targets) == 0: + raise InternalError( + '`item.targets` needs to be a list of length > 0.' + f' Instead, it is {itm.targets}' + ) + + atl.extend(ArgList.fromAstAssign(itm).infoList) + elif isinstance(itm, (ast.AsyncFunctionDef, ast.FunctionDef)): + if treatPropertyMethodsAsClassAttrs and checkIsPropertyMethod(itm): atl.append( Arg( - name=attr.name, - typeHint=unparseAnnotation(attr.returns), + name=itm.name, + typeHint=unparseAnnotation(itm.returns), ) ) - else: - raise InternalError( - f'Unknown type of class attribute: {type(attr)}' - ) + + if not shouldDocumentPrivateClassAttributes: + atl = [_ for _ in atl if not _.name.startswith('_')] return ArgList(infoList=atl) diff --git a/tests/data/edge_cases/12_property_methods_as_class_attr/google.py b/tests/data/edge_cases/12_property_methods_as_class_attr/google.py index 36c5b4f..db714e4 100644 --- a/tests/data/edge_cases/12_property_methods_as_class_attr/google.py +++ b/tests/data/edge_cases/12_property_methods_as_class_attr/google.py @@ -4,6 +4,7 @@ class House: Attributes: price (float): House price + _privateProperty (str): A private property Args: price_0 (float): House price @@ -27,3 +28,7 @@ def price(self, new_price): @price.deleter def price(self): del self._price + + @property + def _privateProperty(self) -> str: + return 'secret' diff --git a/tests/data/edge_cases/13_class_attr_assignments/google.py b/tests/data/edge_cases/13_class_attr_assignments/google.py new file mode 100644 index 0000000..1e1c2dc --- /dev/null +++ b/tests/data/edge_cases/13_class_attr_assignments/google.py @@ -0,0 +1,26 @@ +class MyClass: + """ + My Class + + Attributes: + a1: attr 1 + a2: attr 2 + a3: attr 3 + a4: attr 4 + a5: attr 5 + a6: attr 6 + a7: attr 7 + a8: attr 8 + a9: attr 9 + a10: attr 10 + a11: attr 11 + a12: attr 12 + a13: attr 13 + a14: attr 14 + a15: attr 15 + """ + + a1, a2, a3 = 1, 2, 3 + a4 = a5 = a6 = 3 + a7 = a8 = a9 = a6 == 2 + a10, a11 = a12, a13 = a14, a15 = 5, 6 diff --git a/tests/test_main.py b/tests/test_main.py index 86508c2..a4148f6 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1332,6 +1332,7 @@ def testNonAscii() -> None: 'style': 'google', 'checkClassAttributes': True, 'treatPropertyMethodsAsClassAttributes': True, + 'shouldDocumentPrivateClassAttributes': True, }, [], ), @@ -1341,8 +1342,43 @@ def testNonAscii() -> None: 'style': 'google', 'checkClassAttributes': True, 'treatPropertyMethodsAsClassAttributes': True, + 'shouldDocumentPrivateClassAttributes': False, }, - [], + [ + 'DOC602: Class `House`: Class docstring contains more class attributes than ' + 'in actual class attributes. (Please read ' + 'https://jsh9.github.io/pydoclint/checking_class_attributes.html on how to ' + 'correctly document class attributes.)', + 'DOC603: Class `House`: Class docstring attributes are different from actual ' + 'class attributes. (Or could be other formatting issues: ' + 'https://jsh9.github.io/pydoclint/violation_codes.html#notes-on-doc103 ). ' + 'Arguments in the docstring but not in the actual class attributes: ' + '[_privateProperty: str]. (Please read ' + 'https://jsh9.github.io/pydoclint/checking_class_attributes.html on how to ' + 'correctly document class attributes.)', + ], + ), + ( + '12_property_methods_as_class_attr/google.py', + { + 'style': 'google', + 'checkClassAttributes': True, + 'treatPropertyMethodsAsClassAttributes': False, + 'shouldDocumentPrivateClassAttributes': True, + }, + [ + 'DOC602: Class `House`: Class docstring contains more class attributes than ' + 'in actual class attributes. (Please read ' + 'https://jsh9.github.io/pydoclint/checking_class_attributes.html on how to ' + 'correctly document class attributes.)', + 'DOC603: Class `House`: Class docstring attributes are different from actual ' + 'class attributes. (Or could be other formatting issues: ' + 'https://jsh9.github.io/pydoclint/violation_codes.html#notes-on-doc103 ). ' + 'Arguments in the docstring but not in the actual class attributes: ' + '[_privateProperty: str, price: float]. (Please read ' + 'https://jsh9.github.io/pydoclint/checking_class_attributes.html on how to ' + 'correctly document class attributes.)', + ], ), ( '12_property_methods_as_class_attr/google.py', @@ -1350,6 +1386,7 @@ def testNonAscii() -> None: 'style': 'google', 'checkClassAttributes': True, 'treatPropertyMethodsAsClassAttributes': False, + 'shouldDocumentPrivateClassAttributes': False, }, [ 'DOC602: Class `House`: Class docstring contains more class attributes than ' @@ -1359,12 +1396,20 @@ def testNonAscii() -> None: 'DOC603: Class `House`: Class docstring attributes are different from actual ' 'class attributes. (Or could be other formatting issues: ' 'https://jsh9.github.io/pydoclint/violation_codes.html#notes-on-doc103 ). ' - 'Arguments in the docstring but not in the actual class attributes: [price: ' - 'float]. (Please read ' + 'Arguments in the docstring but not in the actual class attributes: ' + '[_privateProperty: str, price: float]. (Please read ' 'https://jsh9.github.io/pydoclint/checking_class_attributes.html on how to ' 'correctly document class attributes.)', ], ), + ( + '13_class_attr_assignments/google.py', + { + 'style': 'google', + 'checkClassAttributes': True, + }, + [], + ), ], ) def testEdgeCases( @@ -1389,6 +1434,7 @@ def testPlayground() -> None: violations = _checkFile( filename=DATA_DIR / 'playground.py', style='google', + skipCheckingRaises=True, ) expected = [] assert list(map(str, violations)) == expected diff --git a/tests/utils/test_visitor_helper.py b/tests/utils/test_visitor_helper.py index adbdcbc..67346db 100644 --- a/tests/utils/test_visitor_helper.py +++ b/tests/utils/test_visitor_helper.py @@ -1,6 +1,10 @@ +import ast + import pytest +from pydoclint.utils.arg import Arg, ArgList from pydoclint.utils.visitor_helper import ( + extractClassAttributesFromNode, extractReturnTypeFromGenerator, extractYieldTypeFromGeneratorOrIteratorAnnotation, ) @@ -122,3 +126,162 @@ def testExtractReturnTypeFromGenerator( ) -> None: extracted = extractReturnTypeFromGenerator(returnAnnoText) assert extracted == expected + + +@pytest.mark.parametrize( + 'docPriv, treatProp, expected', + [ + ( + True, + True, + ArgList([ + Arg(name='a1', typeHint=''), + Arg(name='attr1', typeHint='int'), + Arg(name='attr2', typeHint='float'), + Arg(name='attr3', typeHint=''), + Arg(name='attr4', typeHint=''), + Arg(name='attr5', typeHint=''), + Arg(name='attr6', typeHint=''), + Arg(name='attr7', typeHint=''), + Arg(name='attr8', typeHint=''), + Arg(name='attr9', typeHint=''), + Arg(name='attr10', typeHint=''), + Arg(name='attr11', typeHint=''), + Arg(name='attr12', typeHint='bool'), + Arg(name='a13', typeHint=''), + Arg(name='a14', typeHint=''), + Arg(name='a15', typeHint=''), + Arg(name='a16', typeHint=''), + Arg(name='a17', typeHint=''), + Arg(name='a18', typeHint=''), + Arg(name='a19', typeHint=''), + Arg(name='a20', typeHint=''), + Arg(name='a21', typeHint=''), + Arg(name='_privAttr1', typeHint='int'), + Arg(name='prop1', typeHint='float | str | dict | None'), + Arg(name='_privProp', typeHint='str'), + ]), + ), + ( + False, + False, + ArgList([ + Arg(name='a1', typeHint=''), + Arg(name='attr1', typeHint='int'), + Arg(name='attr2', typeHint='float'), + Arg(name='attr3', typeHint=''), + Arg(name='attr4', typeHint=''), + Arg(name='attr5', typeHint=''), + Arg(name='attr6', typeHint=''), + Arg(name='attr7', typeHint=''), + Arg(name='attr8', typeHint=''), + Arg(name='attr9', typeHint=''), + Arg(name='attr10', typeHint=''), + Arg(name='attr11', typeHint=''), + Arg(name='attr12', typeHint='bool'), + Arg(name='a13', typeHint=''), + Arg(name='a14', typeHint=''), + Arg(name='a15', typeHint=''), + Arg(name='a16', typeHint=''), + Arg(name='a17', typeHint=''), + Arg(name='a18', typeHint=''), + Arg(name='a19', typeHint=''), + Arg(name='a20', typeHint=''), + Arg(name='a21', typeHint=''), + ]), + ), + ( + True, + False, + ArgList([ + Arg(name='a1', typeHint=''), + Arg(name='attr1', typeHint='int'), + Arg(name='attr2', typeHint='float'), + Arg(name='attr3', typeHint=''), + Arg(name='attr4', typeHint=''), + Arg(name='attr5', typeHint=''), + Arg(name='attr6', typeHint=''), + Arg(name='attr7', typeHint=''), + Arg(name='attr8', typeHint=''), + Arg(name='attr9', typeHint=''), + Arg(name='attr10', typeHint=''), + Arg(name='attr11', typeHint=''), + Arg(name='attr12', typeHint='bool'), + Arg(name='a13', typeHint=''), + Arg(name='a14', typeHint=''), + Arg(name='a15', typeHint=''), + Arg(name='a16', typeHint=''), + Arg(name='a17', typeHint=''), + Arg(name='a18', typeHint=''), + Arg(name='a19', typeHint=''), + Arg(name='a20', typeHint=''), + Arg(name='a21', typeHint=''), + Arg(name='_privAttr1', typeHint='int'), + ]), + ), + ( + False, + True, + ArgList([ + Arg(name='a1', typeHint=''), + Arg(name='attr1', typeHint='int'), + Arg(name='attr2', typeHint='float'), + Arg(name='attr3', typeHint=''), + Arg(name='attr4', typeHint=''), + Arg(name='attr5', typeHint=''), + Arg(name='attr6', typeHint=''), + Arg(name='attr7', typeHint=''), + Arg(name='attr8', typeHint=''), + Arg(name='attr9', typeHint=''), + Arg(name='attr10', typeHint=''), + Arg(name='attr11', typeHint=''), + Arg(name='attr12', typeHint='bool'), + Arg(name='a13', typeHint=''), + Arg(name='a14', typeHint=''), + Arg(name='a15', typeHint=''), + Arg(name='a16', typeHint=''), + Arg(name='a17', typeHint=''), + Arg(name='a18', typeHint=''), + Arg(name='a19', typeHint=''), + Arg(name='a20', typeHint=''), + Arg(name='a21', typeHint=''), + Arg(name='prop1', typeHint='float | str | dict | None'), + ]), + ), + ], +) +def testExtractClassAttributesFromNode( + docPriv: bool, + treatProp: bool, + expected: ArgList, +) -> None: + code: str = """ +class MyClass: + a1 = 1 + attr1: int = 1 + attr2: float = 1.0 + attr3, attr4, attr5 = 1, 2, 3 + attr6 = attr7 = attr8 = attr9 = attr10 = -1 + attr11 = attr1 == 2 + attr12: bool = attr1 == 2 + a13, a14, a15 = a16, a17, a18 = a19, a20, a21 = (1, 2), 3, 4 + _privAttr1: int = 12345 + + @property + def prop1(self) -> float | str | dict | None: + return 2.2 + + @property + def _privProp(self) -> str: + return 'secret' + + def nonProperty(self) -> int: + return 1 +""" + parsed = ast.parse(code) + extracted: ArgList = extractClassAttributesFromNode( + node=parsed.body[0], + shouldDocumentPrivateClassAttributes=docPriv, + treatPropertyMethodsAsClassAttrs=treatProp, + ) + assert extracted == expected