Enhance Linting and Code Coverage Checks with New Scripts and Job Separation #2810
Conversation
WalkthroughThe pull request introduces two new Python scripts and updates the GitHub Actions workflow to enhance code quality checks. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/eslint_disable_check.py (1)
76-77: Combine nested if statements for cleaner logic.You can simplify this by merging the condition that checks if a file ends with ".ts" and the call to
has_eslint_disable.Here’s a possible diff:
- if item.endswith(".ts"): - if has_eslint_disable(item): - print(f"File {item} contains eslint-disable statement. Please remove them and ensure the code adheres to the specified ESLint rules.") - eslint_found = True + if item.endswith(".ts") and has_eslint_disable(item): + print(f"File {item} contains eslint-disable statement. Please remove them and ensure the code adheres to the specified ESLint rules.") + eslint_found = True🧰 Tools
🪛 Ruff (0.8.2)
76-77: Use a single
ifstatement instead of nestedifstatements(SIM102)
.github/workflows/code_coverage_disable_check.py (1)
91-103: Combine nested if statements to reduce complexity.Similar to the ESLint checker script, merging both conditions into a single statement can make the code more concise and maintainable.
Here’s a suggested diff:
- if ( - item.endswith(".ts") - and not item.endswith(".test.ts") - and not item.endswith(".spec.ts") - ): - if has_code_coverage_disable(item): - print(...) - code_coverage_found = True + if ( + item.endswith(".ts") + and not item.endswith(".test.ts") + and not item.endswith(".spec.ts") + and has_code_coverage_disable(item) + ): + print(...) + code_coverage_found = True🧰 Tools
🪛 Ruff (0.8.2)
91-98: Use a single
ifstatement instead of nestedifstatements(SIM102)
93-98: Use a single
ifstatement instead of nestedifstatements(SIM102)
.github/workflows/pull-request.yml (2)
77-77: Quote variables to avoid shell expansion issues.Shellcheck suggests quoting
$CHANGED_FILESto prevent unwanted word splitting. For instance:- run: npx eslint ${CHANGED_FILES} --max-warnings=1500 + run: npx eslint "${CHANGED_FILES}" --max-warnings=1500🧰 Tools
🪛 actionlint (1.7.4)
77-77: shellcheck reported issue in this script: SC2086:info:1:12: Double quote to prevent globbing and word splitting
(shellcheck)
218-218: Remove trailing spaces.YAML linting flags trailing spaces on this line. Consider removing them to keep the file neatly formatted.
- python .github/workflows/code_coverage_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }} + python .github/workflows/code_coverage_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 218-218: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/eslint_disable_check.py(3 hunks).github/workflows/pull-request.yml(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
.github/workflows/eslint_disable_check.py
76-77: Use a single if statement instead of nested if statements
(SIM102)
.github/workflows/code_coverage_disable_check.py
91-98: Use a single if statement instead of nested if statements
(SIM102)
93-98: Use a single if statement instead of nested if statements
(SIM102)
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
77-77: shellcheck reported issue in this script: SC2086:info:1:12: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 218-218: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
.github/workflows/eslint_disable_check.py (3)
7-9: Good clarity in docstring.
These lines effectively communicate that both directories and individual files can now be analyzed. Clear documentation helps maintainers and contributors understand the script usage.
142-145: Validate fallback to directory arguments.
Here, if --files is not provided, you default to --directory. This logic appears sound. Confirm that there are no edge cases where both arguments are empty.
Do you want a script to check for invocations of this file without either argument specified?
147-152: Exit code ensures robust CI checks.
Exiting with error code 1 when an ESLint-disable is found provides a clear CI signal. This matches best practices for fail-fast pipelines.
.github/workflows/code_coverage_disable_check.py (2)
1-10: New file addition is well-documented.
The script's high-level docstring clearly states its purpose and methodology. Good adherence to Python docstring conventions.
133-158: Solid design for script entry point.
Your main() function clearly lays out the usage flow. Handling file vs. directory arguments and performing checks aligns well with the project's coding style.
.github/workflows/pull-request.yml (2)
180-219: Separate jobs for ESLint and coverage checks are well-structured.
Defining dedicated jobs Check-ESlint-Disable and Check-Code-Coverage-Disable isolates responsibilities and enhances maintainability. This is a beneficial addition to the workflow.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 218-218: trailing spaces
(trailing-spaces)
223-223: Good practice to gate final tests on new checks.
Requiring [Code-Quality-Checks, Check-Eslint-Disable, Check-Code-Coverage-Disable] ensures no test runs if essential checks fail, enforcing robust CI gating.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2810 +/- ##
========================================
Coverage 97.72% 97.72%
========================================
Files 364 364
Lines 18545 18545
Branches 2676 2676
========================================
Hits 18123 18123
Misses 417 417
Partials 5 5 ☔ View full report in Codecov by Sentry. |
palisadoes
merged commit f998e32
into
PalisadoesFoundation:develop
|
Add to the |
What kind of change does this PR introduce?
Feature
Issue Number:
Fixes #2806
Summary
Summary by CodeRabbit
New Features
Chores