diff --git a/CHANGELOG.md b/CHANGELOG.md index b2b7470..f4576a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Change Log +## [0.5.5] - 2024-07-15 + +- Fixed + + - Fixed a bug where `a = b = c = 1` style cannot be properly parsed + (https://github.com/jsh9/pydoclint/issues/151) + +- Changed + - Changed the default of `--treat-property-methods-as-class-attributes` to + `False` to restore backward compatibility + ## [0.5.4] - 2024-07-14 - Added diff --git a/docs/config_options.md b/docs/config_options.md index e9e062c..0a0bf1c 100644 --- a/docs/config_options.md +++ b/docs/config_options.md @@ -26,7 +26,7 @@ page: - [13. `--ignore-underscore-args` (shortform: `-iua`, default: `True`)](#13---ignore-underscore-args-shortform--iua-default-true) - [14. `--check-class-attributes` (shortform: `-cca`, default: `True`)](#14---check-class-attributes-shortform--cca-default-true) - [15. `--should-document-private-class-attributes` (shortform: `-sdpca`, default: `False`)](#15---should-document-private-class-attributes-shortform--sdpca-default-false) -- [16. `--treat-property-methods-as-class-attributes` (shortform: `-tpmaca`, default: `True`)](#16---treat-property-methods-as-class-attributes-shortform--tpmaca-default-true) +- [16. `--treat-property-methods-as-class-attributes` (shortform: `-tpmaca`, default: `False`)](#16---treat-property-methods-as-class-attributes-shortform--tpmaca-default-false) - [17. `--baseline`](#17---baseline) - [18. `--generate-baseline` (default: `False`)](#18---generate-baseline-default-false) - [19. `--show-filenames-in-every-violation-message` (shortform: `-sfn`, default: `False`)](#19---show-filenames-in-every-violation-message-shortform--sfn-default-false) @@ -194,13 +194,12 @@ for more instructions. If True, private class attributes (those that start with leading `_`) should be documented. If False, they should not be documented. -## 16. `--treat-property-methods-as-class-attributes` (shortform: `-tpmaca`, default: `True`) +## 16. `--treat-property-methods-as-class-attributes` (shortform: `-tpmaca`, default: `False`) If True, treat `@property` methods as class properties. This means that they need to be documented in the "Attributes" section of the class docstring, and there cannot be any docstring under the @property methods. This option is only -effective when --check-class-attributes is True. We recommend setting both this -option and --check-class-attributes to True. +effective when --check-class-attributes is True. ## 17. `--baseline` diff --git a/pydoclint/flake8_entry.py b/pydoclint/flake8_entry.py index 20f86c8..93b8513 100644 --- a/pydoclint/flake8_entry.py +++ b/pydoclint/flake8_entry.py @@ -183,7 +183,7 @@ def add_options(cls, parser): # noqa: D102 '-tpmaca', '--treat-property-methods-as-class-attributes', action='store', - default='True', + default='False', parse_from_config=True, help=( 'If True, treat @property methods as class properties. This means' diff --git a/pydoclint/main.py b/pydoclint/main.py index 3678874..ef16f5a 100644 --- a/pydoclint/main.py +++ b/pydoclint/main.py @@ -223,7 +223,7 @@ def validateStyleValue( '--treat-property-methods-as-class-attributes', type=bool, show_default=True, - default=True, + default=False, help=( 'If True, treat @property methods as class properties. This means' ' that they need to be documented in the "Attributes" section of' @@ -527,7 +527,7 @@ def _checkPaths( ignoreUnderscoreArgs: bool = True, checkClassAttributes: bool = True, shouldDocumentPrivateClassAttributes: bool = False, - treatPropertyMethodsAsClassAttributes: bool = True, + treatPropertyMethodsAsClassAttributes: bool = False, requireReturnSectionWhenReturningNothing: bool = False, requireYieldSectionWhenYieldingNothing: bool = False, quiet: bool = False, @@ -606,7 +606,7 @@ def _checkFile( ignoreUnderscoreArgs: bool = True, checkClassAttributes: bool = True, shouldDocumentPrivateClassAttributes: bool = False, - treatPropertyMethodsAsClassAttributes: bool = True, + treatPropertyMethodsAsClassAttributes: bool = False, requireReturnSectionWhenReturningNothing: bool = False, requireYieldSectionWhenYieldingNothing: bool = False, ) -> List[Violation]: diff --git a/pydoclint/utils/arg.py b/pydoclint/utils/arg.py index 2427162..c74b2c9 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,26 @@ 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': + """Construct an ArgList from variable declaration/assignment""" + 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..5721bdc 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,11 @@ 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 +120,62 @@ 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 + + Raises + ------ + InternalError + When the length of item.targets is 0 + """ + 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/setup.cfg b/setup.cfg index cf63676..4bd026d 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,6 @@ [metadata] name = pydoclint -version = 0.5.4 +version = 0.5.5 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/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