diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c177f809..50706799 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -27,7 +27,7 @@ jobs: # stop the build if there are Python syntax errors or undefined names flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics --exclude=.git,__pycache__,docs/source/conf.py,old,build,dist,venv,test/resources,.eggs,review.egg-info,.pytest_cache,node_modules # TODO: change max-complexity into 10 after refactoring - flake8 . --count --max-complexity=11 --max-line-length=120 --max-doc-length=120 --ignore=I201,I202,I101,I100,R504,A003,E800,SC200,SC100,E402,W503,WPS --statistics --exclude=.git,__pycache__,docs/source/conf.py,old,build,dist,venv,test/resources,.eggs,review.egg-info,.pytest_cache,node_modules + flake8 . --count --max-complexity=11 --max-line-length=120 --max-doc-length=120 --ignore=I201,I202,I101,I100,R504,A003,E800,SC200,SC100,E402,W503,WPS,C812,H601 --statistics --exclude=.git,__pycache__,docs/source/conf.py,old,build,dist,venv,test/resources,.eggs,review.egg-info,.pytest_cache,node_modules - name: Set up Eslint run: | npm install eslint --save-dev diff --git a/requirements.txt b/requirements.txt index caa6e2e2..e13a08f4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -18,6 +18,10 @@ flake8-spellcheck==0.14.0 mccabe==0.6.1 pep8-naming==0.11.1 wps-light==0.15.2 +flake8-broken-line==0.3.0 +flake8-string-format==0.3.0 +flake8-commas==2.0.0 +cohesion==1.0.0 # extra libraries and frameworks django==3.0.8 diff --git a/src/python/review/inspectors/flake8/.flake8 b/src/python/review/inspectors/flake8/.flake8 index 0ad30a62..6cb0d0cf 100644 --- a/src/python/review/inspectors/flake8/.flake8 +++ b/src/python/review/inspectors/flake8/.flake8 @@ -41,3 +41,14 @@ ignore=W291, # trailing whitespaces WPS527, # Require tuples as arguments for frozenset. # WPS: OOP WPS602, # Forbid @staticmethod decorator. + # flake8-string-format + P101, # format string does contain unindexed parameters + P102, # docstring does contain unindexed parameters + P103, # other string does contain unindexed parameters + F522, # unused named arguments. TODO: Collision with "P302" + F523, # unused positional arguments. TODO: Collision with "P301" + F524, # missing argument. TODO: Collision with "P201" and "P202" + F525, # mixing automatic and manual numbering. TODO: Collision with "P205" + # flake8-commas + C814, # missing trailing comma in Python + \ No newline at end of file diff --git a/src/python/review/inspectors/flake8/flake8.py b/src/python/review/inspectors/flake8/flake8.py index 9784b8ab..c7318d0b 100644 --- a/src/python/review/inspectors/flake8/flake8.py +++ b/src/python/review/inspectors/flake8/flake8.py @@ -1,4 +1,5 @@ import logging +import math import re from pathlib import Path from typing import List @@ -7,7 +8,14 @@ from src.python.review.inspectors.base_inspector import BaseInspector from src.python.review.inspectors.flake8.issue_types import CODE_PREFIX_TO_ISSUE_TYPE, CODE_TO_ISSUE_TYPE from src.python.review.inspectors.inspector_type import InspectorType -from src.python.review.inspectors.issue import BaseIssue, CodeIssue, CyclomaticComplexityIssue, IssueType, IssueData +from src.python.review.inspectors.issue import ( + BaseIssue, + CodeIssue, + CyclomaticComplexityIssue, + IssueType, + IssueData, + CohesionIssue, +) from src.python.review.inspectors.tips import get_cyclomatic_complexity_tip logger = logging.getLogger(__name__) @@ -27,6 +35,7 @@ def inspect(cls, path: Path, config: dict) -> List[BaseIssue]: f'--format={FORMAT}', f'--config={PATH_FLAKE8_CONFIG}', '--max-complexity', '0', + '--cohesion-below', '100', path, ] output = run_in_subprocess(command) @@ -36,12 +45,14 @@ def inspect(cls, path: Path, config: dict) -> List[BaseIssue]: def parse(cls, output: str) -> List[BaseIssue]: row_re = re.compile(r'^(.*):(\d+):(\d+):([A-Z]+\d{3}):(.*)$', re.M) cc_description_re = re.compile(r"'(.+)' is too complex \((\d+)\)") + cohesion_description_re = re.compile(r"class has low \((\d*\.?\d*)%\) cohesion") issues: List[BaseIssue] = [] for groups in row_re.findall(output): description = groups[4] origin_class = groups[3] cc_match = cc_description_re.match(description) + cohesion_match = cohesion_description_re.match(description) file_path = Path(groups[0]) line_no = int(groups[1]) @@ -51,15 +62,20 @@ def parse(cls, output: str) -> List[BaseIssue]: line_number=line_no, column_number=column_number, origin_class=origin_class) - if cc_match is not None: - issue_data['description'] = get_cyclomatic_complexity_tip() - issue_data['cc_value'] = int(cc_match.groups()[1]) - issue_data['type'] = IssueType.CYCLOMATIC_COMPLEXITY + if cc_match is not None: # mccabe: cyclomatic complexity + issue_data[IssueData.DESCRIPTION.value] = get_cyclomatic_complexity_tip() + issue_data[IssueData.CYCLOMATIC_COMPLEXITY.value] = int(cc_match.groups()[1]) + issue_data[IssueData.ISSUE_TYPE.value] = IssueType.CYCLOMATIC_COMPLEXITY issues.append(CyclomaticComplexityIssue(**issue_data)) + elif cohesion_match is not None: # flake8-cohesion + issue_data[IssueData.DESCRIPTION.value] = description # TODO: Add tip + issue_data[IssueData.COHESION_LACK.value] = cls.__get_cohesion_lack(float(cohesion_match.group(1))) + issue_data[IssueData.ISSUE_TYPE.value] = IssueType.COHESION + issues.append(CohesionIssue(**issue_data)) else: issue_type = cls.choose_issue_type(origin_class) - issue_data['type'] = issue_type - issue_data['description'] = description + issue_data[IssueData.ISSUE_TYPE.value] = issue_type + issue_data[IssueData.DESCRIPTION.value] = description issues.append(CodeIssue(**issue_data)) return issues @@ -82,3 +98,14 @@ def choose_issue_type(code: str) -> IssueType: return IssueType.BEST_PRACTICES return issue_type + + @staticmethod + def __get_cohesion_lack(cohesion_percentage: float) -> int: + """ + Converts cohesion percentage to lack of cohesion. + Calculated by the formula: floor(100 - cohesion_percentage). + + :param cohesion_percentage: cohesion set as a percentage. + :return: lack of cohesion + """ + return math.floor(100 - cohesion_percentage) diff --git a/src/python/review/inspectors/flake8/issue_types.py b/src/python/review/inspectors/flake8/issue_types.py index 749d708b..26d5d923 100644 --- a/src/python/review/inspectors/flake8/issue_types.py +++ b/src/python/review/inspectors/flake8/issue_types.py @@ -16,6 +16,17 @@ # builtin naming 'A003': IssueType.BEST_PRACTICES, + # flake8-broken-line + 'N400': IssueType.CODE_STYLE, + + # flake8-commas + "C812": IssueType.CODE_STYLE, + "C813": IssueType.CODE_STYLE, + "C815": IssueType.CODE_STYLE, + "C816": IssueType.CODE_STYLE, + "C818": IssueType.CODE_STYLE, + "C819": IssueType.CODE_STYLE, + # WPS: Naming "WPS117": IssueType.CODE_STYLE, # Forbid naming variables self, cls, or mcs. "WPS125": IssueType.ERROR_PRONE, # Forbid variable or module names which shadow builtin names. @@ -87,6 +98,7 @@ 'B': IssueType.ERROR_PRONE, # flake8-bugbear 'A': IssueType.ERROR_PRONE, # flake8-builtins 'R': IssueType.ERROR_PRONE, # flake8-return + 'P': IssueType.ERROR_PRONE, # flake8-format-string 'E': IssueType.CODE_STYLE, # standard flake8 'W': IssueType.CODE_STYLE, # standard flake8 diff --git a/src/python/review/inspectors/intellij/issue_types/__init__.py b/src/python/review/inspectors/intellij/issue_types/__init__.py index c6f21127..3482d8b4 100644 --- a/src/python/review/inspectors/intellij/issue_types/__init__.py +++ b/src/python/review/inspectors/intellij/issue_types/__init__.py @@ -1,15 +1,21 @@ from typing import Dict -from src.python.review import IssueType -from .java import ISSUE_CLASS_TO_ISSUE_TYPE as \ - JAVA_ISSUE_CLASS_TO_ISSUE_TYPE -from .kotlin import ISSUE_CLASS_TO_ISSUE_TYPE as \ - KOTLIN_ISSUE_CLASS_TO_ISSUE_TYPE -from .python import ISSUE_CLASS_TO_ISSUE_TYPE as \ - PYTHON_ISSUE_CLASS_TO_ISSUE_TYPE +from src.python.review.inspectors.issue import IssueType + +from src.python.review.inspectors.intellij.issue_types.java import ( + ISSUE_CLASS_TO_ISSUE_TYPE as JAVA_ISSUE_CLASS_TO_ISSUE_TYPE, +) + +from src.python.review.inspectors.intellij.issue_types.kotlin import ( + ISSUE_CLASS_TO_ISSUE_TYPE as KOTLIN_ISSUE_CLASS_TO_ISSUE_TYPE, +) + +from src.python.review.inspectors.intellij.issue_types.python import ( + ISSUE_CLASS_TO_ISSUE_TYPE as PYTHON_ISSUE_CLASS_TO_ISSUE_TYPE, +) ISSUE_CLASS_TO_ISSUE_TYPE: Dict[str, IssueType] = { **JAVA_ISSUE_CLASS_TO_ISSUE_TYPE, **PYTHON_ISSUE_CLASS_TO_ISSUE_TYPE, - **KOTLIN_ISSUE_CLASS_TO_ISSUE_TYPE + **KOTLIN_ISSUE_CLASS_TO_ISSUE_TYPE, } diff --git a/src/python/review/inspectors/issue.py b/src/python/review/inspectors/issue.py index 14f0bebb..e4a21798 100644 --- a/src/python/review/inspectors/issue.py +++ b/src/python/review/inspectors/issue.py @@ -45,6 +45,7 @@ class IssueData(Enum): FUNCTION_LEN = 'func_len' BOOL_EXPR_LEN = 'bool_expr_len' CYCLOMATIC_COMPLEXITY = 'cc_value' + COHESION_LACK = 'cohesion_lack' @classmethod def get_base_issue_data_dict(cls, diff --git a/src/python/review/inspectors/parsers/checkstyle_parser.py b/src/python/review/inspectors/parsers/checkstyle_parser.py index 9cf75acb..cae3593f 100644 --- a/src/python/review/inspectors/parsers/checkstyle_parser.py +++ b/src/python/review/inspectors/parsers/checkstyle_parser.py @@ -6,10 +6,24 @@ from src.python.review.common.file_system import get_content_from_file from src.python.review.inspectors.inspector_type import InspectorType -from src.python.review.inspectors.issue import BaseIssue, BoolExprLenIssue, CodeIssue, CyclomaticComplexityIssue, \ - FuncLenIssue, IssueType, LineLenIssue, IssueData -from src.python.review.inspectors.tips import get_bool_expr_len_tip, get_cyclomatic_complexity_tip, get_func_len_tip, \ - get_line_len_tip + +from src.python.review.inspectors.issue import ( + BaseIssue, + BoolExprLenIssue, + CodeIssue, + CyclomaticComplexityIssue, + FuncLenIssue, + IssueType, + LineLenIssue, + IssueData, +) + +from src.python.review.inspectors.tips import ( + get_bool_expr_len_tip, + get_cyclomatic_complexity_tip, + get_func_len_tip, + get_line_len_tip, +) logger = logging.getLogger(__name__) diff --git a/src/python/review/inspectors/springlint/springlint.py b/src/python/review/inspectors/springlint/springlint.py index 18f9d6b0..14ce8c89 100644 --- a/src/python/review/inspectors/springlint/springlint.py +++ b/src/python/review/inspectors/springlint/springlint.py @@ -9,11 +9,30 @@ from src.python.review.common.subprocess_runner import run_in_subprocess from src.python.review.inspectors.base_inspector import BaseInspector from src.python.review.inspectors.inspector_type import InspectorType -from src.python.review.inspectors.issue import BaseIssue, ChildrenNumberIssue, ClassResponseIssue, CodeIssue, \ - CohesionIssue, \ - CouplingIssue, InheritanceIssue, IssueType, MethodNumberIssue, WeightedMethodIssue, IssueData -from src.python.review.inspectors.tips import get_child_number_tip, get_class_coupling_tip, get_class_response_tip, \ - get_cohesion_tip, get_inheritance_depth_tip, get_method_number_tip, get_weighted_method_tip + +from src.python.review.inspectors.issue import ( + BaseIssue, + ChildrenNumberIssue, + ClassResponseIssue, + CodeIssue, + CohesionIssue, + CouplingIssue, + InheritanceIssue, + IssueType, + MethodNumberIssue, + WeightedMethodIssue, + IssueData, +) + +from src.python.review.inspectors.tips import ( + get_child_number_tip, + get_class_coupling_tip, + get_class_response_tip, + get_cohesion_tip, + get_inheritance_depth_tip, + get_method_number_tip, + get_weighted_method_tip, +) PATH_TOOLS_SPRINGLINT_FILES = Path(__file__).parent / 'files' PATH_SPRINGLINT_JAR = PATH_TOOLS_SPRINGLINT_FILES / 'springlint-0.6.jar' diff --git a/src/python/review/inspectors/tips.py b/src/python/review/inspectors/tips.py index c1d3fba1..5211b569 100644 --- a/src/python/review/inspectors/tips.py +++ b/src/python/review/inspectors/tips.py @@ -1,24 +1,32 @@ def get_bool_expr_len_tip() -> str: - return 'Too long boolean expression. ' \ - 'Try to split it into smaller expressions.' + return ( + 'Too long boolean expression. ' + 'Try to split it into smaller expressions.' + ) def get_func_len_tip() -> str: - return 'Too long function. ' \ - 'Try to split it into smaller functions / methods.' \ - 'It will make your code easy to understand and less error prone.' + return ( + 'Too long function. ' + 'Try to split it into smaller functions / methods. ' + 'It will make your code easy to understand and less error prone.' + ) def get_line_len_tip() -> str: - return 'Too long line. ' \ - 'Try to split it into smaller lines.' \ - 'It will make your code easy to understand.' + return ( + 'Too long line. ' + 'Try to split it into smaller lines. ' + 'It will make your code easy to understand.' + ) def get_cyclomatic_complexity_tip() -> str: - return 'Too complex function. You can figure out how to simplify this code ' \ - 'or split it into a set of small functions / methods. ' \ - 'It will make your code easy to understand and less error prone.' + return ( + 'Too complex function. You can figure out how to simplify this code ' + 'or split it into a set of small functions / methods. ' + 'It will make your code easy to understand and less error prone.' + ) def add_complexity_tip(description: str) -> str: @@ -31,8 +39,10 @@ def add_complexity_tip(description: str) -> str: def get_inheritance_depth_tip() -> str: - return 'Too deep inheritance tree is complicated to understand. ' \ - 'Try to reduce it (maybe you should use a composition instead).' + return ( + 'Too deep inheritance tree is complicated to understand. ' + 'Try to reduce it (maybe you should use a composition instead).' + ) # This issue will not be reported at this version @@ -41,14 +51,18 @@ def get_child_number_tip() -> str: def get_weighted_method_tip() -> str: - return 'The number of methods and their complexity may be too hight. ' \ - 'It may require too much time and effort to develop and maintain the class.' + return ( + 'The number of methods and their complexity may be too hight. ' + 'It may require too much time and effort to develop and maintain the class.' + ) def get_class_coupling_tip() -> str: - return 'The class seems to depend on too many other classes. ' \ - 'Increased coupling increases interclass dependencies, ' \ - 'making the code less modular and less suitable for reuse and testing.' + return ( + 'The class seems to depend on too many other classes. ' + 'Increased coupling increases interclass dependencies, ' + 'making the code less modular and less suitable for reuse and testing.' + ) # This issue will not be reported at this version @@ -57,12 +71,16 @@ def get_cohesion_tip() -> str: def get_class_response_tip() -> str: - return 'The class have too many methods that can potentially ' \ - 'be executed in response to a single message received by an object of that class. ' \ - 'The larger the number of methods that can be invoked from a class, ' \ - 'the greater the complexity of the class' + return ( + 'The class have too many methods that can potentially ' + 'be executed in response to a single message received by an object of that class. ' + 'The larger the number of methods that can be invoked from a class, ' + 'the greater the complexity of the class' + ) def get_method_number_tip() -> str: - return 'The file has too many methods inside and is complicated to understand. ' \ - 'Consider its decomposition to smaller classes.' + return ( + 'The file has too many methods inside and is complicated to understand. ' + 'Consider its decomposition to smaller classes.' + ) diff --git a/src/python/review/quality/evaluate_quality.py b/src/python/review/quality/evaluate_quality.py index 3d78e392..d7ee0211 100644 --- a/src/python/review/quality/evaluate_quality.py +++ b/src/python/review/quality/evaluate_quality.py @@ -4,26 +4,43 @@ from src.python.review.inspectors.issue import IssueType from src.python.review.quality.model import Quality, Rule from src.python.review.quality.rules.best_practices_scoring import ( - BestPracticesRule, LANGUAGE_TO_BEST_PRACTICES_RULE_CONFIG + BestPracticesRule, + LANGUAGE_TO_BEST_PRACTICES_RULE_CONFIG, +) +from src.python.review.quality.rules.boolean_length_scoring import ( + BooleanExpressionRule, + LANGUAGE_TO_BOOLEAN_EXPRESSION_RULE_CONFIG, ) -from src.python.review.quality.rules.boolean_length_scoring import BooleanExpressionRule, \ - LANGUAGE_TO_BOOLEAN_EXPRESSION_RULE_CONFIG from src.python.review.quality.rules.class_response_scoring import LANGUAGE_TO_RESPONSE_RULE_CONFIG, ResponseRule from src.python.review.quality.rules.code_style_scoring import CodeStyleRule, LANGUAGE_TO_CODE_STYLE_RULE_CONFIG from src.python.review.quality.rules.coupling_scoring import CouplingRule, LANGUAGE_TO_COUPLING_RULE_CONFIG -from src.python.review.quality.rules.cyclomatic_complexity_scoring import CyclomaticComplexityRule, \ - LANGUAGE_TO_CYCLOMATIC_COMPLEXITY_RULE_CONFIG +from src.python.review.quality.rules.cyclomatic_complexity_scoring import ( + CyclomaticComplexityRule, + LANGUAGE_TO_CYCLOMATIC_COMPLEXITY_RULE_CONFIG, +) from src.python.review.quality.rules.error_prone_scoring import ErrorProneRule, LANGUAGE_TO_ERROR_PRONE_RULE_CONFIG -from src.python.review.quality.rules.function_length_scoring import FunctionLengthRule, \ - LANGUAGE_TO_FUNCTION_LENGTH_RULE_CONFIG -from src.python.review.quality.rules.inheritance_depth_scoring import InheritanceDepthRule, \ - LANGUAGE_TO_INHERITANCE_DEPTH_RULE_CONFIG +from src.python.review.quality.rules.function_length_scoring import ( + FunctionLengthRule, + LANGUAGE_TO_FUNCTION_LENGTH_RULE_CONFIG, +) +from src.python.review.quality.rules.inheritance_depth_scoring import ( + InheritanceDepthRule, + LANGUAGE_TO_INHERITANCE_DEPTH_RULE_CONFIG, +) from src.python.review.quality.rules.line_len_scoring import LANGUAGE_TO_LINE_LENGTH_RULE_CONFIG, LineLengthRule -from src.python.review.quality.rules.method_number_scoring import LANGUAGE_TO_METHOD_NUMBER_RULE_CONFIG, \ - MethodNumberRule -from src.python.review.quality.rules.weighted_methods_scoring import LANGUAGE_TO_WEIGHTED_METHODS_RULE_CONFIG, \ - WeightedMethodsRule +from src.python.review.quality.rules.method_number_scoring import ( + LANGUAGE_TO_METHOD_NUMBER_RULE_CONFIG, + MethodNumberRule, +) +from src.python.review.quality.rules.weighted_methods_scoring import ( + LANGUAGE_TO_WEIGHTED_METHODS_RULE_CONFIG, + WeightedMethodsRule, +) from src.python.review.reviewers.utils.code_statistics import CodeStatistics +from src.python.review.quality.rules.cohesion_scoring import ( + LANGUAGE_TO_COHESION_RULE_CONFIG, + CohesionRule, +) def __get_available_rules(language: Language) -> List[Rule]: @@ -37,7 +54,8 @@ def __get_available_rules(language: Language) -> List[Rule]: MethodNumberRule(LANGUAGE_TO_METHOD_NUMBER_RULE_CONFIG[language]), CouplingRule(LANGUAGE_TO_COUPLING_RULE_CONFIG[language]), ResponseRule(LANGUAGE_TO_RESPONSE_RULE_CONFIG[language]), - WeightedMethodsRule(LANGUAGE_TO_WEIGHTED_METHODS_RULE_CONFIG[language]) + WeightedMethodsRule(LANGUAGE_TO_WEIGHTED_METHODS_RULE_CONFIG[language]), + CohesionRule(LANGUAGE_TO_COHESION_RULE_CONFIG[language]), ] diff --git a/src/python/review/quality/model.py b/src/python/review/quality/model.py index ae71f211..741c761b 100644 --- a/src/python/review/quality/model.py +++ b/src/python/review/quality/model.py @@ -1,4 +1,5 @@ import abc +import textwrap from enum import Enum, unique from functools import total_ordering from typing import List @@ -80,8 +81,12 @@ def __str__(self): message_deltas_part = '' if self.quality_type != QualityType.EXCELLENT: - message_next_level_part = f'Next level: {self.next_quality_type.value}\n' \ - f'Next level requirements:\n' + message_next_level_part = textwrap.dedent( + f"""\ + Next level: {self.next_quality_type.value} + Next level requirements: + """ + ) for rule in self.next_level_requirements: message_deltas_part += f'{rule.rule_type.value}: {rule.next_level_delta}\n' diff --git a/src/python/review/quality/rules/cohesion_scoring.py b/src/python/review/quality/rules/cohesion_scoring.py new file mode 100644 index 00000000..c73a426b --- /dev/null +++ b/src/python/review/quality/rules/cohesion_scoring.py @@ -0,0 +1,71 @@ +from dataclasses import dataclass +from typing import Optional + +from src.python.review.common.language import Language +from src.python.review.inspectors.issue import IssueType +from src.python.review.quality.model import Rule, QualityType + + +@dataclass +class CohesionRuleConfig: + cohesion_lack_bad: int + cohesion_lack_moderate: int + cohesion_lack_good: int + + +# TODO: Need testing +# Cohesion plugin by default only shows issues where cohesion is less than 50%. +# Therefore cohesion_lack_bad = 50. The other levels are set in steps of 20%. +common_cohesion_rule_config = CohesionRuleConfig( + cohesion_lack_bad=50, + cohesion_lack_moderate=30, + cohesion_lack_good=10, +) + +LANGUAGE_TO_COHESION_RULE_CONFIG = { + Language.JAVA: common_cohesion_rule_config, + Language.PYTHON: common_cohesion_rule_config, + Language.KOTLIN: common_cohesion_rule_config, + Language.JS: common_cohesion_rule_config, +} + + +class CohesionRule(Rule): + def __init__(self, config: CohesionRuleConfig): + self.config = config + self.rule_type = IssueType.COHESION + self.cohesion_lack: Optional[int] = None + + def apply(self, cohesion_lack: int): + self.cohesion_lack = cohesion_lack + if cohesion_lack > self.config.cohesion_lack_bad: + self.quality_type = QualityType.BAD + self.next_level_delta = cohesion_lack - self.config.cohesion_lack_bad + elif cohesion_lack > self.config.cohesion_lack_moderate: + self.quality_type = QualityType.MODERATE + self.next_level_delta = cohesion_lack - self.config.cohesion_lack_moderate + elif cohesion_lack > self.config.cohesion_lack_good: + self.quality_type = QualityType.GOOD + self.next_level_delta = cohesion_lack - self.config.cohesion_lack_good + else: + self.quality_type = QualityType.EXCELLENT + self.next_level_delta = 0 + self.next_level_type = self.__get_next_quality_type() + + def __get_next_quality_type(self) -> QualityType: + if self.quality_type == QualityType.BAD: + return QualityType.MODERATE + elif self.quality_type == QualityType.MODERATE: + return QualityType.GOOD + return QualityType.EXCELLENT + + def merge(self, other: 'CohesionRule') -> 'CohesionRule': + config = CohesionRuleConfig( + min(self.config.cohesion_lack_bad, other.config.cohesion_lack_bad), + min(self.config.cohesion_lack_moderate, other.config.cohesion_lack_moderate), + min(self.config.cohesion_lack_good, other.config.cohesion_lack_good), + ) + result_rule = CohesionRule(config) + result_rule.apply(max(self.cohesion_lack, other.cohesion_lack)) + + return result_rule diff --git a/src/python/review/reviewers/utils/code_statistics.py b/src/python/review/reviewers/utils/code_statistics.py index b0523355..41dc25a2 100644 --- a/src/python/review/reviewers/utils/code_statistics.py +++ b/src/python/review/reviewers/utils/code_statistics.py @@ -16,6 +16,7 @@ class CodeStatistics: method_number: int max_cyclomatic_complexity: int + max_cohesion_lack: int max_func_len: int max_bool_expr_len: int @@ -38,6 +39,7 @@ def issue_type_to_statistics_dict(self) -> Dict[IssueType, int]: IssueType.METHOD_NUMBER: self.method_number, IssueType.CYCLOMATIC_COMPLEXITY: self.max_cyclomatic_complexity, + IssueType.COHESION: self.max_cohesion_lack, IssueType.FUNC_LEN: self.max_func_len, IssueType.BOOL_EXPR_LEN: self.max_bool_expr_len, @@ -45,7 +47,7 @@ def issue_type_to_statistics_dict(self) -> Dict[IssueType, int]: IssueType.INHERITANCE_DEPTH: self.inheritance_depth, IssueType.COUPLING: self.coupling, IssueType.CLASS_RESPONSE: self.class_response, - IssueType.WEIGHTED_METHOD: self.weighted_method_complexities + IssueType.WEIGHTED_METHOD: self.weighted_method_complexities, } @@ -82,6 +84,7 @@ def gather_code_statistics(issues: List[BaseIssue], path: Path) -> CodeStatistic bool_expr_lens = __get_max_measure_by_issue_type(IssueType.BOOL_EXPR_LEN, issues) func_lens = __get_max_measure_by_issue_type(IssueType.FUNC_LEN, issues) cyclomatic_complexities = __get_max_measure_by_issue_type(IssueType.CYCLOMATIC_COMPLEXITY, issues) + cohesion_lacks = __get_max_measure_by_issue_type(IssueType.COHESION, issues) # Actually, we expect only one issue with each of the following metrics. inheritance_depths = __get_max_measure_by_issue_type(IssueType.INHERITANCE_DEPTH, issues) @@ -97,6 +100,7 @@ def gather_code_statistics(issues: List[BaseIssue], path: Path) -> CodeStatistic max_bool_expr_len=bool_expr_lens, max_func_len=func_lens, n_line_len=issue_type_counter[IssueType.LINE_LEN], + max_cohesion_lack=cohesion_lacks, max_cyclomatic_complexity=cyclomatic_complexities, inheritance_depth=inheritance_depths, class_response=class_responses, @@ -104,5 +108,5 @@ def gather_code_statistics(issues: List[BaseIssue], path: Path) -> CodeStatistic weighted_method_complexities=weighted_method_complexities, method_number=method_numbers, total_lines=__get_total_lines(path), - code_style_lines=get_code_style_lines(issues) + code_style_lines=get_code_style_lines(issues), ) diff --git a/src/python/review/reviewers/utils/issues_filter.py b/src/python/review/reviewers/utils/issues_filter.py index 9ac9b0b6..ee9be2a1 100644 --- a/src/python/review/reviewers/utils/issues_filter.py +++ b/src/python/review/reviewers/utils/issues_filter.py @@ -11,6 +11,7 @@ from src.python.review.quality.rules.inheritance_depth_scoring import LANGUAGE_TO_INHERITANCE_DEPTH_RULE_CONFIG from src.python.review.quality.rules.method_number_scoring import LANGUAGE_TO_METHOD_NUMBER_RULE_CONFIG from src.python.review.quality.rules.weighted_methods_scoring import LANGUAGE_TO_WEIGHTED_METHODS_RULE_CONFIG +from src.python.review.quality.rules.cohesion_scoring import LANGUAGE_TO_COHESION_RULE_CONFIG def __get_issue_type_to_low_measure_dict(language: Language) -> Dict[IssueType, int]: @@ -22,7 +23,8 @@ def __get_issue_type_to_low_measure_dict(language: Language) -> Dict[IssueType, IssueType.METHOD_NUMBER: LANGUAGE_TO_METHOD_NUMBER_RULE_CONFIG[language].method_number_good, IssueType.COUPLING: LANGUAGE_TO_COUPLING_RULE_CONFIG[language].coupling_moderate, IssueType.CLASS_RESPONSE: LANGUAGE_TO_RESPONSE_RULE_CONFIG[language].response_good, - IssueType.WEIGHTED_METHOD: LANGUAGE_TO_WEIGHTED_METHODS_RULE_CONFIG[language].weighted_methods_good + IssueType.WEIGHTED_METHOD: LANGUAGE_TO_WEIGHTED_METHODS_RULE_CONFIG[language].weighted_methods_good, + IssueType.COHESION: LANGUAGE_TO_COHESION_RULE_CONFIG[language].cohesion_lack_good, } @@ -38,7 +40,7 @@ def filter_low_measure_issues(issues: List[BaseIssue], issue_type_to_low_measure_dict = __get_issue_type_to_low_measure_dict(language) # Disable this types of issue, requires further investigation. - ignored_issues = [IssueType.COHESION, IssueType.CHILDREN_NUMBER] + ignored_issues = [IssueType.CHILDREN_NUMBER] return list(filter( lambda issue: issue.type not in ignored_issues and __more_than_low_measure(issue, diff --git a/src/python/review/run_tool.py b/src/python/review/run_tool.py index fdbb4b67..d0ba0891 100644 --- a/src/python/review/run_tool.py +++ b/src/python/review/run_tool.py @@ -13,8 +13,13 @@ from src.python.review.application_config import ApplicationConfig, LanguageVersion from src.python.review.inspectors.inspector_type import InspectorType from src.python.review.logging_config import logging_config -from src.python.review.reviewers.perform_review import OutputFormat, PathNotExists, perform_and_print_review, \ - UnsupportedLanguage + +from src.python.review.reviewers.perform_review import ( + OutputFormat, + PathNotExists, + perform_and_print_review, + UnsupportedLanguage, +) logger = logging.getLogger(__name__) diff --git a/test/python/inspectors/conftest.py b/test/python/inspectors/conftest.py index 615f95dc..b2f8773c 100644 --- a/test/python/inspectors/conftest.py +++ b/test/python/inspectors/conftest.py @@ -73,6 +73,7 @@ class IssuesTestInfo: n_cc: int = 0 n_bool_expr_len: int = 0 n_other_complexity: int = 0 + n_cohesion: int = 0 def gather_issues_test_info(issues: List[BaseIssue]) -> IssuesTestInfo: @@ -85,7 +86,8 @@ def gather_issues_test_info(issues: List[BaseIssue]) -> IssuesTestInfo: n_func_len=counter[IssueType.FUNC_LEN], n_cc=counter[IssueType.CYCLOMATIC_COMPLEXITY], n_bool_expr_len=counter[IssueType.BOOL_EXPR_LEN], - n_other_complexity=counter[IssueType.COMPLEXITY] + n_other_complexity=counter[IssueType.COMPLEXITY], + n_cohesion=counter[IssueType.COHESION], ) diff --git a/test/python/inspectors/test_flake8_inspector.py b/test/python/inspectors/test_flake8_inspector.py index 3b21c051..c4750ace 100644 --- a/test/python/inspectors/test_flake8_inspector.py +++ b/test/python/inspectors/test_flake8_inspector.py @@ -1,4 +1,5 @@ import pytest +from textwrap import dedent from src.python.review.common.language import Language from src.python.review.inspectors.flake8.flake8 import Flake8Inspector @@ -12,11 +13,11 @@ ('case1_simple_valid_program.py', 0), ('case2_boolean_expressions.py', 8), ('case3_redefining_builtin.py', 2), - ('case4_naming.py', 10), + ('case4_naming.py', 11), ('case5_returns.py', 1), ('case6_unused_variables.py', 3), ('case8_good_class.py', 0), - ('case7_empty_lines.py', 0), + ('case7_empty_lines.py', 2), ('case10_unused_variable_in_loop.py', 1), ('case13_complex_logic.py', 12), ('case13_complex_logic_2.py', 3), @@ -28,6 +29,10 @@ ('case19_bad_indentation.py', 3), ('case21_imports.py', 2), ('case25_django.py', 0), + ('case31_line_break.py', 11), + ('case32_string_format.py', 34), + ('case33_commas.py', 14), + ('case34_cohesion.py', 1), ] @@ -52,11 +57,11 @@ def test_file_with_issues(file_name: str, n_issues: int): n_cc=8, n_other_complexity=2)), ('case3_redefining_builtin.py', IssuesTestInfo(n_error_prone=2)), - ('case4_naming.py', IssuesTestInfo(n_code_style=7, n_best_practices=3, n_cc=5)), + ('case4_naming.py', IssuesTestInfo(n_code_style=7, n_best_practices=3, n_cc=5, n_cohesion=1)), ('case6_unused_variables.py', IssuesTestInfo(n_best_practices=3, n_cc=1)), - ('case8_good_class.py', IssuesTestInfo(n_cc=1)), - ('case7_empty_lines.py', IssuesTestInfo(n_cc=5)), + ('case8_good_class.py', IssuesTestInfo(n_cc=1, n_cohesion=1)), + ('case7_empty_lines.py', IssuesTestInfo(n_cc=5, n_cohesion=2)), ('case10_unused_variable_in_loop.py', IssuesTestInfo(n_best_practices=1, n_cc=1)), ('case13_complex_logic.py', IssuesTestInfo(n_cc=5, n_other_complexity=10)), @@ -64,6 +69,12 @@ def test_file_with_issues(file_name: str, n_issues: int): ('case14_returns_errors.py', IssuesTestInfo(n_best_practices=1, n_error_prone=3, n_cc=4)), + ('case31_line_break.py', IssuesTestInfo(n_best_practices=1, + n_code_style=10, + n_cc=1)), + ('case32_string_format.py', IssuesTestInfo(n_error_prone=28, n_other_complexity=6)), + ('case33_commas.py', IssuesTestInfo(n_code_style=14, n_cc=4)), + ('case34_cohesion.py', IssuesTestInfo(n_cc=6, n_cohesion=2)), ] @@ -81,9 +92,12 @@ def test_file_with_issues_info(file_name: str, expected_issues_info: IssuesTestI def test_parse(): file_name = 'test.py' - output = ('test.py:1:11:W602:test 1\n' - 'test.py:2:12:E703:test 2\n' - 'test.py:3:13:SC200:test 3') + output = f"""\ + {file_name}:1:11:W602:test 1 + {file_name}:2:12:E703:test 2 + {file_name}:3:13:SC200:test 3 + """ + output = dedent(output) issues = Flake8Inspector.parse(output) diff --git a/test/python/inspectors/test_pylint_inspector.py b/test/python/inspectors/test_pylint_inspector.py index 55ce551a..e142e8aa 100644 --- a/test/python/inspectors/test_pylint_inspector.py +++ b/test/python/inspectors/test_pylint_inspector.py @@ -1,3 +1,5 @@ +import textwrap + import pytest from src.python.review.inspectors.issue import IssueType @@ -46,8 +48,11 @@ def test_file_with_issues(file_name: str, n_issues: int): def test_parse(): file_name = 'test.py' - output = 'test.py:1:11:R0123:test 1\n' \ - 'test.py:2:12:C1444:test 2' + output = f"""\ + {file_name}:1:11:R0123:test 1 + {file_name}:2:12:C1444:test 2 + """ + output = textwrap.dedent(output) issues = PylintInspector.parse(output) diff --git a/test/python/inspectors/test_python_ast.py b/test/python/inspectors/test_python_ast.py index 09192cd3..fed3c542 100644 --- a/test/python/inspectors/test_python_ast.py +++ b/test/python/inspectors/test_python_ast.py @@ -3,8 +3,13 @@ import pytest from src.python.review.inspectors.inspector_type import InspectorType -from src.python.review.inspectors.pyast.python_ast import BoolExpressionLensGatherer, FunctionLensGatherer, \ - PythonAstInspector + +from src.python.review.inspectors.pyast.python_ast import ( + BoolExpressionLensGatherer, + FunctionLensGatherer, + PythonAstInspector, +) + from test.python.inspectors import PYTHON_DATA_FOLDER, PYTHON_AST_DATA_FOLDER from test.python.inspectors.conftest import use_file_metadata diff --git a/test/resources/inspectors/python/case31_line_break.py b/test/resources/inspectors/python/case31_line_break.py new file mode 100644 index 00000000..ba718ea8 --- /dev/null +++ b/test/resources/inspectors/python/case31_line_break.py @@ -0,0 +1,54 @@ +# Wrong +from math import exp, \ + log + +# Correct +from math import ( + sin, + cos, +) + + +# Wrong +def do_something_wrong(x: float, y: float): + if sin(x) == cos(y) \ + and exp(x) == log(y): + print("Do not do that!") + + +print(do_something_wrong(1, 2)) + +# Wrong +wrong_string = "Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. " \ + + "Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, " \ + + "nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. " \ + + "Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, " \ + + "vulputate eget, arcu. In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo. " \ + + "Nullam dictum felis eu pede mollis pretium. Integer tincidunt. Cras dapibus. " \ + + "Vivamus elementum semper nisi. Aenean vulputate eleifend tellus. Aenean leo ligula, " \ + + "porttitor eu, consequat vitae, eleifend ac, enim. Aliquam lorem ante, dapibus in, " \ + + "viverra quis, feugiat a," +print(wrong_string) + +# Correct +correct_string = ("Lorem ipsum dolor sit amet, consectetuer adipiscing elit. " + + "Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis " + + "parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, " + + "pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, " + + "aliquet nec, vulputate eget, arcu. In enim justo, rhoncus ut, imperdiet a, " + + "venenatis vitae, justo. Nullam dictum felis eu pede mollis pretium. Integer tincidunt. " + + "Cras dapibus. Vivamus elementum semper nisi. Aenean vulputate eleifend tellus. " + + "Aenean leo ligula, porttitor eu, consequat vitae, eleifend ac, enim. " + + "Aliquam lorem ante, dapibus in, viverra quis, feugiat a," + ) +print(correct_string) + +other_correct_string = """ + Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec euismod vitae libero ut consequat. + Fusce quis ultrices sem, vitae viverra mi. Praesent fermentum quam ac volutpat condimentum. + Proin mauris orci, molestie vel fermentum vel, consectetur vel metus. Quisque vitae mollis magna. + In hac habitasse platea dictumst. Pellentesque sed diam eget dolor ultricies faucibus id sed quam. + Nam risus erat, varius ut risus a, tincidunt vulputate magna. Etiam lacinia a eros non faucibus. + In facilisis tempor nisl, sit amet feugiat lacus viverra quis. +""" +print(other_correct_string) diff --git a/test/resources/inspectors/python/case32_string_format.py b/test/resources/inspectors/python/case32_string_format.py new file mode 100644 index 00000000..bee24195 --- /dev/null +++ b/test/resources/inspectors/python/case32_string_format.py @@ -0,0 +1,143 @@ +hello_world = "Hello, World!" +hello = "Hello" +world = "World" +some_list = [hello, world] +some_dict = {"hello": hello, "world": world} + +# ----------------------------------------------------------------------------------- + +# Correct +print("{0}!".format(hello_world)) + +# Correct +print("{}!".format(hello_world)) + +# Correct +print("{0}, {1}!".format(hello, world)) + +# Correct +print("{0}, {1}!".format(*some_list)) + +# Correct +print("{1}, {0}!".format(world, hello)) + +# Correct +print("{1}, {0}!".format(*some_list)) + +# Correct +print("{}, {}!".format(hello, world)) + +# Correct +print("{}, {}!".format(*some_list)) + +# Correct +print("{hello}, {world}!".format(hello=hello, world=world)) + +# Correct +print("{hello}, {world}!".format(**some_dict)) + +# ----------------------------------------------------------------------------------- + +# Correct +print(str.format("{0}!", hello_world)) + +# Correct +print(str.format("{}!", hello_world)) + +# Correct +print(str.format("{0}, {1}!", hello, world)) + +# Correct +print(str.format("{0}, {1}!", *some_list)) + +# Correct +print(str.format("{1}, {0}!", world, hello)) + +# Correct +print(str.format("{1}, {0}!", *some_list)) + +# Correct +print(str.format("{}, {}!", hello, world)) + +# Correct +print(str.format("{}, {}!", *some_list)) + +# Correct +print(str.format("{hello}, {world}!", hello=hello, world=world)) + +# Correct +print(str.format("{hello}, {world}!", **some_dict)) + +# ----------------------------------------------------------------------------------- + +# Wrong: P-201 +print("{0}, {1}!".format(hello)) + +# Wrong: P-201 +print("{}, {}!".format(hello)) + +# Wrong: P-201 +print(str.format("{0}, {1}!", hello)) + +# Wrong: P-201 +print(str.format("{}, {}!", hello)) + +# ----------------------------------------------------------------------------------- + +# Wrong: P-202 +print("{hello}, {world}!".format(hello=hello)) + +# Wrong: P-202 +print(str.format("{hello}, {world}!", hello=hello)) + +# ----------------------------------------------------------------------------------- + +# Wrong: P-203 +print("{0}!".format(**some_dict)) + +# Wrong: P-203 +print("{}!".format(**some_dict)) + +# Wrong: P-203 +print(str.format("{0}!", **some_dict)) + +# Wrong: P-203 +print(str.format("{}!", **some_dict)) + +# ----------------------------------------------------------------------------------- + +# Wrong: P-204 +print("{hello_world}!".format(*some_list)) + +# Wrong: P-204 +print(str.format("{hello_world}!", *some_list)) + +# ----------------------------------------------------------------------------------- + +# Wrong: P-205 +print("{}, {0}!".format(hello, world)) + +# Wrong: P-205 +print(str.format("{}, {0}!", hello, world)) + +# ----------------------------------------------------------------------------------- + +# Wrong: P-301 +print("{0}".format(hello_world, hello, world)) + +# Wrong: P-301 +print("{}".format(hello_world, hello, world)) + +# Wrong: P-301 +print(str.format("{0}", hello_world, hello, world)) + +# Wrong: P-301 +print(str.format("{}", hello_world, hello, world)) + +# ----------------------------------------------------------------------------------- + +# Wrong: P-302 +print("{hello_world}".format(hello_world=hello_world, hello=hello, world=world)) + +# Wrong: P-302 +print(str.format("{hello_world}", hello_world=hello_world, hello=hello, world=world)) diff --git a/test/resources/inspectors/python/case33_commas.py b/test/resources/inspectors/python/case33_commas.py new file mode 100644 index 00000000..6af55963 --- /dev/null +++ b/test/resources/inspectors/python/case33_commas.py @@ -0,0 +1,153 @@ +# Wrong C-812 +from math import ( + log +) + +# Correct +from math import ( + sin, +) + +# Wrong C-812 +bad_multiline_dict = { + "first": 1, + "second": 2 +} + +# Correct +good_multiline_dict = { + "first": 1, + "second": 2, +} + +# Wrong C-812 +bad_multiline_list = [ + 1, + 2, + 3 +] + +# Correct +good_multiline_list = [ + 1, + 2, + 3, +] + +# Wrong C-812 +bad_multiline_tuple = ( + 3, + 4 +) + +good_multiline_tuple = ( + 3, + 4, +) + + +# Wrong C-812 +def bad_function( + a, + b +): + return log(a, b) + + +bad_function( + 1, + 2 +) + +bad_function( + a=1, + b=2 +) + + +# Correct +def good_function( + a, + b, +): + return a + sin(b) + + +good_function( + 1, + 2, +) + +good_function( + a=1, + b=2, +) + +# Wrong: C-813 +print( + "Hello", + "World" +) + +# Correct +print( + "Hello", + "World", +) + + +# Wrong: C-816 +def bad_function_with_unpacking( + a, + b, + **kwargs +): + pass + + +# Correct +def good_function_with_unpacking( + a, + b, + **kwargs, +): + pass + + +# Wrong: C-815 +good_function_with_unpacking( + 1, + 2, + **good_multiline_dict +) + +# Correct +good_function_with_unpacking( + 1, + 2, + **good_multiline_dict, +) + +# Wrong: C-818 +bad_comma = 1, + +# Correct +good_comma = (1,) + +# Wrong: C-819 +bad_list = [1, 2, 3, ] + +# Correct: +good_list = [1, 2, 3] + +# Wrong: C-819 +bad_dict = {"1": 1, "2": 2, "3": 3, } + +# Correct: +good_dict = {"1": 1, "2": 2, "3": 3} + +# Wrong: C-819 +bad_tuple = (1, 2, 3,) + +# Correct +good_tuple = (1, 2, 3) diff --git a/test/resources/inspectors/python/case34_cohesion.py b/test/resources/inspectors/python/case34_cohesion.py new file mode 100644 index 00000000..8102675a --- /dev/null +++ b/test/resources/inspectors/python/case34_cohesion.py @@ -0,0 +1,28 @@ +from math import sqrt + + +class BadClass: + def __init__(self, x: int, y: int): + self.x = x + self.y = y + + @staticmethod + def length(x: int, y: int) -> float: + return sqrt(x ** 2 + y ** 2) + + @staticmethod + def dot(self_x: int, self_y: int, other_x: int, other_y: int) -> int: + return self_x * other_x + self_y * other_y + + +class GoodClass(object): + def __init__(self, x: int, y: int): + self.x = x + self.y = y + + @property + def length(self) -> float: + return sqrt(self.dot(self.x, self.y)) + + def dot(self, other_x: int, other_y: int) -> int: + return self.x * other_x + self.y * other_y diff --git a/whitelist.txt b/whitelist.txt index 8f29cf65..c2473e99 100644 --- a/whitelist.txt +++ b/whitelist.txt @@ -61,3 +61,5 @@ lcom noc nom wmc +multiline +sqrt