Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat (HIS): add cyclomatic complexity check #49

Closed

Conversation

AfonsoSantos96
Copy link
Member

This PR introduces the feature of checking the cyclomatic complexity of the HIS metrics in the ci.

Next is a list of the HIS metrics, indicating which checks are currently being supported, and what tools are being used to check each metric.

Metric Decription Supported Tool
CALLING Number of callers (<5)
CALLS Number of called functions (<7)
COMF Ratio of comments to statements (>=0.2)
CYC Cyclomatic Complexity (<=10) x pmccabe
GOTO Number of Goto (=0)
LEVEL Number of nested conditions (<=4)
PARAMS Number of function parameters (<=5)
PATH Number of Paths (<80)
REC Recursive Functions (=0)
RETURN Number of returns in function (<=1)
STMT Number of statements in function (<=50)
VOCF Vocabulary of function (<=4)

The MISRA-related metrics are addressed by the separate misra check:

  • NOMV: Violations of the HIS subset of MISRA C rules
  • NOMVP: Violations of the HIS subset of MISRA C rules by rule

The following metrics are currently not addressed since they are related to
project evolution and cannot be evaluated in a single commit/snapshot:

  • SCHG: Number of statements changed in project
  • SDEL: Number of statements deleted in project
  • SI: Stability index of project
  • SNEW: Number of statements added in project

Checklist:

  • The changes generate no new warnings when building the project. If so, I have justified above.
  • I have run the CI checkers before submitting the PR to avoid unnecessary runs of the workflow.

Copy link
Member

@danielRep danielRep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As previously pointed before on the first PR, some comments still stand and are not being addressed (that's why I didn't reviewed the main script). Please review according to the comments that me and @josecm pointed on the time:

Before running each metric, I think an output information should notice the user which metric is being "in the moment" checked.

  • The script process all metrics file by file. This approach makes the output a mess. I will prefer that all files are processed metric by metric to have a sense of what is currently being tested.

I agree.

  • In a case a specific file isn't complaint with some of the metrics, will the name of the file be printed? I can see that with cflow, but I'm asking if there are other cases with other tools where that isn't going to happen

Also agree. The metrics should always hide the output from the tools. I guess this is being done for some but not all. Are we intercepting stderr as well as stdout? On errors we should print the name of the file, the metric, the value calculated as well as the threshold.

I think it would be better to use regex instead of ad hoc parsing to process the output of each tool.

Comment on lines +296 to +297
his-check:
@./his_checker.py $(_his_files)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this from a root of a repo with the ci module, this won't run. Besides follow the same sintax as "license-check"

Suggested change
his-check:
@./his_checker.py $(_his_files)
his_check_script:=$(ci_dir)/his_checker.py
his-check:
@$(his_check_script) $(_his_files)

Comment on lines +49 to +51
# Set execution permissions on the HIS metrics file
RUN chmod +x his_metrics.py

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed. You should push the file with +x permissions.

import argparse
import os

CYCLOMATIC_TRESHOLD = 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CYCLOMATIC_TRESHOLD = 10
CYCLOMATIC_THRESHOLD = 10

@danielRep danielRep self-assigned this Sep 1, 2023
for num in range(len(sline)-1):
fields = sline[num].split('\t')
colony_count = int(fields[0])
if colony_count > CYCLOMATIC_TRESHOLD:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if colony_count > CYCLOMATIC_TRESHOLD:
if colony_count > CYCLOMATIC_THRESHOLD:

fields = sline[num].split('\t')
colony_count = int(fields[0])
if colony_count > CYCLOMATIC_TRESHOLD:
print("Complexity exceeded (max." + str(CYCLOMATIC_TRESHOLD) + "): "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print("Complexity exceeded (max." + str(CYCLOMATIC_TRESHOLD) + "): "
print("Complexity exceeded (max." + str(CYCLOMATIC_THRESHOLD) + "): "

@danielRep danielRep changed the base branch from wip/his-metrics to wip/his_intro September 28, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants