Add code coverage disable check to GitHub workflows #2701
Conversation
WalkthroughThe pull request introduces a new Python script Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 1
🧹 Nitpick comments (5)
.github/workflows/code_coverage_disable_check.py (4)
37-37: Consider enhancing the regex pattern for better precision.The current pattern could be split into separate patterns for inline and block comments for better maintainability and precision.
- code_coverage_disable_pattern = re.compile(r'(?://\s*istanbul\s+ignore(?:-next-line|-line)?|/\*\s*istanbul\s+ignore\s*(?:next|-line)\s*\*/)', re.IGNORECASE) + INLINE_IGNORE_PATTERN = re.compile(r'//\s*istanbul\s+ignore(?:-next-line|-line)?', re.IGNORECASE) + BLOCK_IGNORE_PATTERN = re.compile(r'/\*\s*istanbul\s+ignore\s*(?:next|-line)\s*\*/', re.IGNORECASE) + return bool(INLINE_IGNORE_PATTERN.search(content) or BLOCK_IGNORE_PATTERN.search(content))
63-63: Fix unused loop variable.The
dirsvariable inos.walkis unused.- for root, dirs, files in os.walk(directory): + for root, _dirs, files in os.walk(directory):🧰 Tools
🪛 Ruff (0.8.2)
63-63: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
71-74: Consider refactoring duplicate code coverage check.The setup.ts check duplicates the logic from
has_code_coverage_disable. Consider reusing the existing logic in a more modular way.- setup_path = os.path.join(directory, 'setup.ts') - if os.path.exists(setup_path) and has_code_coverage_disable(setup_path): - print(f'Setup file {setup_path} contains code coverage disable statement.') - code_coverage_found = True + def check_specific_file(filepath, file_type): + if os.path.exists(filepath) and has_code_coverage_disable(filepath): + print(f'{file_type} file {filepath} contains code coverage disable statement.') + return True + return False + + setup_path = os.path.join(directory, 'setup.ts') + code_coverage_found = check_specific_file(setup_path, 'Setup') or code_coverage_found
94-120: Consider enhancing the main function for better testability.While the implementation is solid, consider:
- Separating the execution logic from the main function
- Adding more detailed error messages
+def run_coverage_check(directories): + """Execute the coverage check logic. + + Args: + directories (list): List of directories to check. + + Returns: + bool: True if successful, False if issues found. + """ + print("Checking directories:", directories) + code_coverage_found = check_code_coverage(directories) + + if code_coverage_found: + print("ERROR: Code coverage disable statements found in the following files.") + return False + + print("SUCCESS: No code coverage disable statements found.") + return True + def main(): """Execute the script's main functionality.""" args = arg_parser_resolver() - print("Directories to check: ", args.directory) - - # Check code coverage in the specified directory - code_coverage_found = check_code_coverage(args.directory) - - if code_coverage_found: - print("Code coverage disable check failed. Exiting with error.") + if not run_coverage_check(args.directory): sys.exit(1) - - print("Code coverage disable check completed successfully.").github/workflows/pull-request.yml (1)
190-205: Fix trailing whitespace and consider job dependencies.The job configuration looks good but has a minor formatting issue and could benefit from explicit dependencies.
- Remove trailing whitespace after line 201
- Consider adding this job as a dependency for the Test-Application job, similar to Check-ESlint-Disable
Check-Code-Coverage-Disable: name: Check for code coverage disable runs-on: ubuntu-latest steps: - name: Checkout code uses: actions/checkout@v4 - name: Set up Python uses: actions/setup-python@v5 with: - python-version: 3.9 + python-version: 3.9 - name: Run Python script run: | python .github/workflows/code_coverage_disable_check.py🧰 Tools
🪛 yamllint (1.35.1)
[error] 201-201: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 201-201: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
.github/workflows/code_coverage_disable_check.py
63-63: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
🔇 Additional comments (2)
.github/workflows/code_coverage_disable_check.py (1)
78-92: LGTM! Well-structured argument parser.
The argument parser is well-implemented with clear documentation and appropriate defaults.
.github/workflows/pull-request.yml (1)
205-205: 🛠️ Refactor suggestion
Consider adding job dependency.
The Test-Application job should wait for this check to complete.
Test-Application:
name: Test Application
runs-on: ubuntu-latest
- needs: [Code-Quality-Checks, Check-ESlint-Disable]
+ needs: [Code-Quality-Checks, Check-ESlint-Disable, Check-Code-Coverage-Disable]Likely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2701 +/- ##
=====================================================
+ Coverage 72.46% 87.79% +15.33%
=====================================================
Files 296 313 +17
Lines 7357 8269 +912
Branches 1606 1874 +268
=====================================================
+ Hits 5331 7260 +1929
+ Misses 1766 736 -1030
- Partials 260 273 +13 ☔ View full report in Codecov by Sentry. |
|
@palisadoes eslint_disable_check.py does not check for multi-line comments used to disable ESLint, and it cannot be run for multiple directories using the CLI. Can I fix these in this pr? |
|
…d documentation standards.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/eslint_disable_check.py (1)
50-56:⚠️ Potential issueEnhance error handling for CI environment
In a CI environment, it would be more useful to fail fast rather than silently continuing when file errors occur.
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1).github/workflows/code_coverage_disable_check.py (1)
49-57:⚠️ Potential issueEnhance error handling for CI environment
For consistency with the eslint checker, improve error handling to fail fast in CI environment.
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1)
🧹 Nitpick comments (4)
.github/workflows/eslint_disable_check.py (2)
40-44: Consider using verbose regex mode for better readabilityThe regex pattern is complex and could benefit from using verbose mode (
re.VERBOSE) to improve maintainability.eslint_disable_pattern = re.compile( - r"""\/\/\s*eslint-disable(?:-next-line - |-line)?[^\n]*|\/\*\s*eslint-disable[^\*]*\*\/""", - re.IGNORECASE, + r""" + (?x) # Enable verbose mode + (?: + \/\/\s* # Single line comment + eslint-disable # eslint-disable + (?:-next-line|-line)? # Optional modifiers + [^\n]* # Rest of the line + | # OR + \/\*\s* # Multi-line comment start + eslint-disable # eslint-disable + [^\*]* # Content + \*\/ # Comment end + ) + """, + re.IGNORECASE,
94-100: Consider extracting setup file check to a shared utilityThe setup file check logic is duplicated in both scripts. Consider extracting it to a shared utility function.
Create a new file
.github/workflows/utils.py:def check_setup_file(directory, check_func, message_prefix): """Check setup.ts file in the given directory. Args: directory (str): Directory containing setup.ts check_func (callable): Function to check the file message_prefix (str): Prefix for the error message Returns: bool: True if issue found, False otherwise """ setup_path = os.path.join(directory, "setup.ts") if os.path.exists(setup_path) and check_func(setup_path): print(f"{message_prefix} {setup_path}") return True return False.github/workflows/code_coverage_disable_check.py (2)
39-43: Consider using verbose regex mode for better readabilitySimilar to the eslint checker, the regex pattern would be more maintainable using verbose mode.
code_coverage_disable_pattern = re.compile( - r"""(?://\s*istanbul\s+ignore(?:-next-line|-line)? - |/\*\s*istanbul\s+ignore\s*(?:next|-line)\s*\*/)""", - re.IGNORECASE, + r""" + (?x) # Enable verbose mode + (?: + //\s* # Single line comment + istanbul\s+ignore # istanbul ignore + (?:-next-line|-line)? # Optional modifiers + | # OR + /\*\s* # Multi-line comment start + istanbul\s+ignore # istanbul ignore + \s*(?:next|-line) # Modifiers + \s*\*/ # Comment end + ) + """, + re.IGNORECASE,
1-155: Consider consolidating the checker scriptsBoth scripts share very similar structure and patterns. Consider consolidating them into a single, configurable checker script to reduce code duplication and improve maintainability.
Create a new file
.github/workflows/code_checker.pythat can handle both eslint and coverage checks:class CodeChecker: def __init__(self, check_type): self.patterns = { 'eslint': r'...', # eslint pattern 'coverage': r'...' # coverage pattern } self.check_type = check_type def check_files(self, directories): """Generic file checking logic""" pass # Usage: # python code_checker.py --type eslint --directory src/ # python code_checker.py --type coverage --directory src/🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/eslint_disable_check.py(6 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/eslint_disable_check.py (3)
40-44: Add documentation for the regex pattern.The regex pattern is complex and would benefit from inline documentation explaining each part of the pattern.
Consider adding a comment explaining the pattern:
eslint_disable_pattern = re.compile( + # Matches both single-line comments (// eslint-disable...) + # and multi-line comments (/* eslint-disable... */) r"""\/\/\s*eslint-disable(?:-next-line |-line)?[^\n]*|\/\*\s*eslint-disable[^\*]*\*\/""", re.IGNORECASE, )
73-93: Simplify nested loops and fix string formatting.The nested loops could be simplified, and the print statements have inconsistent formatting.
Consider this refactoring:
for directory in directories: if not os.path.exists(directory): - print( - f"""Error: The specified directory '{directory}' does not - exist.""" - ) + print(f"Error: The specified directory '{directory}' does not exist.") sys.exit(1) for root, _, files in os.walk(directory): - for file_name in files: - if ( - file_name.endswith(".tsx") - and not file_name.endswith(".test.tsx") - ): - file_path = os.path.join(root, file_name) - if has_eslint_disable(file_path): - print( - f"""File {file_path} contains eslint-disable - statement.""" - ) - eslint_found = True + tsx_files = [f for f in files if f.endswith(".tsx") and not f.endswith(".test.tsx")] + for file_name in tsx_files: + file_path = os.path.join(root, file_name) + if has_eslint_disable(file_path): + print(f"File {file_path} contains eslint-disable statement.") + eslint_found = True
94-100: Fix string formatting in setup.ts check.The print statement for setup.ts has inconsistent formatting.
setup_path = os.path.join(directory, "setup.ts") if os.path.exists(setup_path) and has_eslint_disable(setup_path): - print( - f"""Setup file {setup_path} contains eslint-disable - statement.""" - ) + print(f"Setup file {setup_path} contains eslint-disable statement.") eslint_found = True
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/eslint_disable_check.py(5 hunks)
🔇 Additional comments (4)
.github/workflows/eslint_disable_check.py (4)
7-8: LGTM! Documentation updates are clear and accurate.
The documentation has been properly updated to reflect the script's functionality and compliance standards.
Also applies to: 20-20
50-56: LGTM! Improved error handling with specific exceptions.
The error handling has been enhanced with specific cases for FileNotFoundError and PermissionError, providing better feedback.
115-118: LGTM! CLI argument handling properly supports multiple directories.
The implementation of multiple directory support is well done with clear help messages.
40-44: Verify the regex pattern against various eslint-disable formats.
Let's verify that the regex pattern correctly catches all variations of eslint-disable comments.
✅ Verification successful
The regex pattern correctly matches all eslint-disable comment variations in the codebase
The verification shows that the regex pattern in eslint_disable_check.py correctly matches all variations of eslint-disable comments found in the codebase, including:
- Single-line comments with
eslint-disable-next-line - Multi-line comments with various formats:
/* eslint-disable *//* eslint-disable @typescript-eslint/no-explicit-any *//*eslint-disable*/(without spaces)
The pattern's flexibility in handling whitespace and optional rule specifications makes it robust for the codebase's actual usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test the regex pattern against various eslint-disable formats
# Create a test file with different eslint-disable patterns
cat << 'EOF' > test_eslint.tsx
// eslint-disable-next-line
// eslint-disable-line
// eslint-disable
/* eslint-disable */
/* eslint-disable-next-line */
EOF
# Run the script on the test file
python3 .github/workflows/eslint_disable_check.py --directory .
# Clean up
rm test_eslint.tsx
Length of output: 191
Script:
#!/bin/bash
# Let's verify the regex pattern by searching for actual eslint-disable patterns in the codebase
# This will help us understand if the pattern matches real-world usage
# Search for single-line eslint-disable comments
echo "=== Single-line eslint-disable patterns ==="
rg "//.*eslint-disable.*" -g "!.github/*" --no-heading
echo -e "\n=== Multi-line eslint-disable patterns ==="
# Search for multi-line eslint-disable comments
rg "/\*.*eslint-disable.*\*/" -g "!.github/*" --no-heading
Length of output: 839
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/eslint_disable_check.py (3)
40-44: Improve regex pattern readability and efficiency.The regex pattern could be more readable and maintainable with proper formatting and the use of re.VERBOSE flag.
Consider this improvement:
- eslint_disable_pattern = re.compile( - r"""\/\/\s*eslint-disable(?:-next-line - |-line)?[^\n]*|\/\*\s*eslint-disable[^\*]*\*\/""", - re.IGNORECASE, - ) + eslint_disable_pattern = re.compile( + r''' + # Match single-line comments + \/\/\s*eslint-disable(?:-next-line|-line)?[^\n]* + | + # Match multi-line comments + \/\*\s*eslint-disable[^\*]*\*\/ + ''', + re.IGNORECASE | re.VERBOSE, + )
73-79: Consider checking all directories upfront.Moving the directory existence check outside the main loop would be more efficient and provide better user feedback.
Consider this approach:
+ # Validate all directories upfront + for directory in directories: + if not os.path.exists(directory): + print(f"Error: The specified directory '{directory}' does not exist.") + sys.exit(1) + for directory in directories: - if not os.path.exists(directory): - print( - f"""Error: The specified directory '{directory}' does not - exist.""" - ) - sys.exit(1) for root, _, files in os.walk(directory):
90-93: Fix inconsistent string formatting.The multi-line string literals have inconsistent indentation which could affect readability.
Consider using consistent string formatting:
- print( - f"""File {file_path} contains eslint-disable - statement.""" - ) + print(f"File {file_path} contains eslint-disable statement.") - print( - f"""Setup file {setup_path} contains eslint-disable - statement.""" - ) + print(f"Setup file {setup_path} contains eslint-disable statement.")Also applies to: 98-101
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/eslint_disable_check.py(5 hunks)
🔇 Additional comments (3)
.github/workflows/eslint_disable_check.py (3)
7-8: LGTM! Documentation is comprehensive and follows standards.
The docstring clearly explains the methodology and compliance with Python coding standards, including the addition of Python Black.
Also applies to: 20-20
50-56: LGTM! Error handling is comprehensive.
The error handling is well-structured with specific exceptions for different scenarios, providing clear feedback to users.
117-120: LGTM! CLI argument handling is well implemented.
The implementation correctly supports multiple directories with clear help messages and proper default values.
|
@palisadoes Can you review this pr? |
There was a problem hiding this comment.
- Why isn't this logic sued for eslint_disable-check.py?
- It's inconsistent
- Also, it checks all files edited, and the logic assumes each is a directory.
- The original script worked as is. You shouldn't need to to do this change.
- Please verify this by including a file in the PR with the statements to see whether either script detects the presence. Do this for both statement types.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pull-request.yml (1)
213-213:⚠️ Potential issueAdd code coverage check as a dependency
The Test-Application job should wait for the code coverage check to complete before running.
Update the
needsarray in theTest-Applicationjob:Test-Application: name: Test Application runs-on: ubuntu-latest - needs: [Code-Quality-Checks, Check-ESlint-Disable] + needs: [Code-Quality-Checks, Check-ESlint-Disable, Check-Code-Coverage-Disable]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
210-210: shellcheck reported issue in this script: SC2086:info:1:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 205-205: trailing spaces
(trailing-spaces)
[error] 208-208: trailing spaces
(trailing-spaces)
|
@palisadoes Job: Code-Quality-Checks → Step: Check for linting errors in modified files Job: Check-ESlint-Disable → Step: Check for eslint-disable However, it appears to check all the files in the repository instead of only the files submitted in the pull request. Can you clarify this behavior? |
palisadoes
added
the
ignore-sensitive-files-pr
|
…ctories to Python script
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/code_coverage_disable_check.py (2)
94-103: Make setup.ts path configuration more flexible.The setup.ts path is hardcoded and might not work for all project structures.
Consider making the setup file path configurable through command-line arguments:
+ parser.add_argument( + "--setup-file", + type=str, + default="setup.ts", + help="Path to setup.ts file relative to each directory", + )Then update the setup file check:
- setup_path = os.path.join(directory, "setup.ts") + setup_path = os.path.join(directory, args.setup_file)
144-144: Remove debug print statement.The print statement appears to be used for debugging and should be removed in production code.
- print(args.directory)
📜 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/pull-request.yml(1 hunks)src/App.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/App.tsx
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
193-193: could not parse as YAML: yaml: line 193: did not find expected '-' indicator
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 203-203: syntax error: expected , but found '?'
(syntax)
[error] 225-225: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
.github/workflows/code_coverage_disable_check.py (2)
82-85: 🛠️ Refactor suggestion
Include .ts files in the coverage check.
The current implementation only checks .tsx files, potentially missing coverage disable statements in regular .ts files.
Apply this diff to include .ts files:
if (
- file_name.endswith(".tsx")
- and not file_name.endswith(".test.tsx")
+ (file_name.endswith(".tsx") or file_name.endswith(".ts"))
+ and not (file_name.endswith(".test.tsx") or file_name.endswith(".test.ts"))
):Likely invalid or redundant comment.
45-58: 🛠️ Refactor suggestion
Enhance error handling for better visibility in CI environment.
The current error handling silently returns False for all error cases, which could mask issues in a CI environment.
Apply this diff to improve error handling:
try:
with open(file_path, "r", encoding="utf-8") as file:
content = file.read()
return bool(code_coverage_disable_pattern.search(content))
except FileNotFoundError:
- print(f"File not found: {file_path}")
- return False
+ print(f"ERROR: File not found: {file_path}")
+ sys.exit(1)
except PermissionError:
- print(f"Permission denied: {file_path}")
- return False
+ print(f"ERROR: Permission denied: {file_path}")
+ sys.exit(1)
except (IOError, OSError) as e:
- print(f"Error reading file {file_path}: {e}")
- return False
+ print(f"ERROR: Failed to read file {file_path}")
+ print(f"Cause: {str(e)}")
+ sys.exit(1)Likely invalid or redundant comment.
.github/workflows/pull-request.yml (2)
242-242: 🛠️ Refactor suggestion
Add code coverage check as a dependency.
The Test-Application job should wait for the code coverage check to complete.
Test-Application:
name: Test Application
runs-on: ubuntu-latest
- needs: [Code-Quality-Checks, Check-ESlint-Disable]
+ needs: [Code-Quality-Checks, Check-ESlint-Disable, Check-Code-Coverage-Disable]Likely invalid or redundant comment.
239-240:
Fix potential shell injection vulnerability.
The script passes the directory argument without proper quoting.
- python .github/workflows/code_coverage_disable_check.py --directory $NEAREST_DIRS
+ python .github/workflows/code_coverage_disable_check.py --directory "${NEAREST_DIRS}"Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
190-241: Consider caching Python dependenciesTo improve workflow execution time, consider caching pip dependencies using actions/cache.
Add this step after Python setup:
- name: Cache pip dependencies uses: actions/cache@v3 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('.github/workflows/code_coverage_disable_check.py') }} restore-keys: | ${{ runner.os }}-pip-🧰 Tools
🪛 actionlint (1.7.4)
203-203: shellcheck reported issue in this script: SC2086:info:11:23: Double quote to prevent globbing and word splitting
(shellcheck)
203-203: shellcheck reported issue in this script: SC2076:warning:14:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:27:38: Double quote to prevent globbing and word splitting
(shellcheck)
238-238: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 225-225: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
203-203: shellcheck reported issue in this script: SC2086:info:11:23: Double quote to prevent globbing and word splitting
(shellcheck)
203-203: shellcheck reported issue in this script: SC2076:warning:14:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:27:38: Double quote to prevent globbing and word splitting
(shellcheck)
238-238: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 225-225: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/pull-request.yml (3)
190-196: Add code coverage check as a dependency for Test-Application job
The Test-Application job should wait for the code coverage check to complete.
197-200: LGTM! Changed files retrieval is properly implemented.
232-241:
Fix script execution and improve error handling
The script execution needs to be improved for security and reliability:
- Fix shell globbing vulnerability
- Add explicit error handling
Apply this diff:
- name: Run Python script
run: |
- # Access the nearest_dirs environment variable
- python .github/workflows/code_coverage_disable_check.py --directory $NEAREST_DIRS
+ if ! python .github/workflows/code_coverage_disable_check.py --directory "${NEAREST_DIRS}"; then
+ echo "Error: Code coverage disable check failed"
+ exit 1
+ fiLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
238-238: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
190-238: Optimize job execution and integrationConsider the following improvements to the job:
- Add conditional execution based on file types (e.g., only run on
.tsxand.tsfiles)- Integrate with other code quality checks by adding it to the
Code-Quality-ChecksjobExample implementation:
Check-Code-Coverage-Disable: name: Check for code coverage disable runs-on: ubuntu-latest + if: | + contains(steps.changed-files.outputs.all_changed_files, '.tsx') || + contains(steps.changed-files.outputs.all_changed_files, '.ts') steps: # ... existing steps ...🧰 Tools
🪛 actionlint (1.7.4)
203-203: shellcheck reported issue in this script: SC2076:warning:11:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:24:38: Double quote to prevent globbing and word splitting
(shellcheck)
235-235: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 222-222: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
203-203: shellcheck reported issue in this script: SC2076:warning:11:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:24:38: Double quote to prevent globbing and word splitting
(shellcheck)
235-235: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 222-222: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/pull-request.yml (3)
190-193: Add code coverage check as a dependency for Test-Application job
The Test-Application job should wait for the code coverage check to complete before running.
234-237:
Fix shell globbing vulnerability and improve error handling
The Python script execution has several issues that need to be addressed:
- Shell globbing vulnerability in directory argument
- Missing error handling for script execution
Apply this diff to fix the issues:
- name: Run Python script
run: |
# Access the nearest_dirs environment variable
- python .github/workflows/code_coverage_disable_check.py --directory $NEAREST_DIRS
+ if ! python .github/workflows/code_coverage_disable_check.py --directory "${NEAREST_DIRS}"; then
+ echo "Error: Code coverage disable check failed"
+ exit 1
+ fiLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
235-235: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
201-227:
Fix shell script security vulnerabilities
The directory identification script has several issues that need to be addressed:
- Unquoted variables leading to word splitting
- Incorrect regex comparison syntax
- Missing input validation
- Undefined
repo_rootvariable used in path construction
Apply this diff to fix the security issues:
- name: Identify nearest changed directories
id: nearest_dirs
run: |
+ if [ -z "${{ steps.changed-files.outputs.all_changed_files }}" ]; then
+ echo "No files changed"
+ exit 0
+ fi
+
# Initialize an empty string to store the nearest directories
nearest_dirs=""
# Loop through each changed file to extract the nearest directory
- for file in ${{ steps.changed-files.outputs.all_changed_files }}
+ IFS=$'\n'
+ for file in $(echo "${{ steps.changed-files.outputs.all_changed_files }}" | tr ',' '\n')
do
# Get the full directory from the file path
folder=$(dirname "$file")
- full_path="$repo_root/$folder"
+ full_path="${folder}"
# Add the folder to the nearest_dirs string if not already present
- if [[ ! " $nearest_dirs " =~ " $full_path " ]]; then
+ if [[ ! " ${nearest_dirs} " =~ [[:space:]]${full_path}[[:space:]] ]]; then
- if [ -z "$nearest_dirs" ]; then
+ if [ -z "${nearest_dirs}" ]; then
- nearest_dirs="$full_path"
+ nearest_dirs="${full_path}"
else
- nearest_dirs="$nearest_dirs,$full_path"
+ nearest_dirs="${nearest_dirs},${full_path}"
fi
fi
done
# Output the nearest changed directories
- echo "Nearest changed directories: $nearest_dirs"
+ echo "Nearest changed directories: ${nearest_dirs}"
# Set the nearest directories as a GitHub environment variable
- echo "NEAREST_DIRS=$nearest_dirs" >> $GITHUB_ENV
+ echo "NEAREST_DIRS=${nearest_dirs}" >> "${GITHUB_ENV}"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
203-203: shellcheck reported issue in this script: SC2076:warning:11:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:24:38: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 222-222: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/setupTests.ts (1)
6-6: Avoid disabling ESLint globally.This directive applies to the entire file and may mask other issues. Rather than disabling ESLint for the entire file, consider disabling specific rules for selected lines where necessary.
.github/workflows/eslint_disable_check.py (2)
75-77: Combine nestedifstatements to improve clarity.The nested checks for
if item.endswith(".ts") or item.endswith(".tsx"):followed by anotherif has_eslint_disable(item):can be combined for simplicity.Apply this diff:
- if item.endswith(".ts") or item.endswith(".tsx"): - if has_eslint_disable(item): - print(f"File {item} contains eslint-disable statement.") - eslint_found = True + if (item.endswith(".ts") or item.endswith(".tsx")) and has_eslint_disable(item): + print(f"File {item} contains eslint-disable statement.") + eslint_found = True🧰 Tools
🪛 Ruff (0.8.2)
75-76: Use a single
ifstatement instead of nestedifstatements(SIM102)
139-139: Consider removing leftover print statements for production code.The
print(files_or_directories)call might be for debugging and could be removed or replaced with a structured logger to reduce console clutter..github/workflows/pull-request.yml (1)
212-212: Remove trailing spaces.This line contains trailing whitespace. Consider removing it to keep the file clean and conform to linting standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 212-212: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/eslint_disable_check.py(3 hunks).github/workflows/pull-request.yml(1 hunks)src/setupTests.ts(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 212-212: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
.github/workflows/eslint_disable_check.py
75-76: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
192-193: 🛠️ Refactor suggestion
Double-quote environment variables to prevent word splitting.
When passing file paths to the Python script, wrap the variable in quotes to avoid shell word splitting, especially if any filenames contain spaces.
- python .github/workflows/eslint_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }}
+ python .github/workflows/eslint_disable_check.py --files "${{ steps.changed-files.outputs.all_changed_files }}"Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/code_coverage_disable_check.py (1)
28-56:⚠️ Potential issueImprove error handling for CI environment
The current error handling approach silently returns False for all errors, which could mask real issues in a CI environment. This needs to be fixed to ensure proper error propagation.
Apply this diff to improve error handling:
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1).github/workflows/pull-request.yml (2)
181-193:⚠️ Potential issueFix shell globbing vulnerability
The current script execution is vulnerable to shell globbing and word splitting.
- name: Run Python script run: | - python .github/workflows/eslint_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }} + python .github/workflows/eslint_disable_check.py --files "${{ steps.changed-files.outputs.all_changed_files }}"
194-212:⚠️ Potential issueFix shell globbing vulnerability in code coverage check
The script execution is vulnerable to shell globbing and word splitting.
- name: Run Python script run: | - 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] 212-212: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (3)
.github/workflows/code_coverage_disable_check.py (2)
99-123: Add input validation for CLI argumentsConsider adding validation to ensure provided paths exist and are accessible.
+def validate_path(path): + """Validate if path exists and is accessible.""" + if not os.path.exists(path): + print(f"ERROR: Path does not exist: {path}") + sys.exit(1) + if not os.access(path, os.R_OK): + print(f"ERROR: Path is not readable: {path}") + sys.exit(1) + return path def arg_parser_resolver(): parser = argparse.ArgumentParser() parser.add_argument( "--directory", type=str, nargs="+", + type=validate_path, default=[os.getcwd()], help="""One or more directories to check for code coverage disable statements (default: current directory).""", ) parser.add_argument( "--files", type=str, nargs="+", + type=validate_path, default=[], help="""One or more files to check directly for code coverage disable statements (default: check directories).""", )
142-143: Remove debug print statementThe print statement on line 143 appears to be for debugging purposes and should be removed.
files_or_dirs = args.files if args.files else args.directory - print(files_or_dirs).github/workflows/pull-request.yml (1)
217-217: Fix YAML formattingAdd a space after the comma in the needs array.
- needs: [Code-Quality-Checks, Check-ESlint-Disable,Check-Code-Coverage-Disable] + needs: [Code-Quality-Checks, Check-ESlint-Disable, Check-Code-Coverage-Disable]🧰 Tools
🪛 yamllint (1.35.1)
[warning] 217-217: too few spaces after comma
(commas)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 212-212: trailing spaces
(trailing-spaces)
[warning] 217-217: too few spaces after comma
(commas)
🪛 Ruff (0.8.2)
.github/workflows/code_coverage_disable_check.py
86-92: Use a single if statement instead of nested if statements
(SIM102)
88-92: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
🔇 Additional comments (1)
.github/workflows/code_coverage_disable_check.py (1)
1-26: LGTM! Well-documented script with appropriate imports.
The docstring clearly explains the script's purpose and compliance standards. The imports are appropriate for the required functionality.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
|
@palisadoes I have made the necessary changes. I have also added a file to check whether these tests checks the presence of disable statements. Can you review the logic ? Once you review it, I will make the changes so that all python scripts in the PR are compliant with pylint, pycodestyle, pydocstyle, flake8 and black. |
What kind of change does this PR introduce?
New Script:
Added .github/workflows/code_coverage_disable_check.py script.
Recursively scans specified directories for istanbul ignore patterns in .tsx and setup.ts files (excluding .test.tsx).
Supports multiple directories via --directory CLI argument with a default directory option.
GitHub Workflow:
Integrated the code_coverage_disable_check.py into the workflow.
Automatically runs during PR checks to ensure no code coverage disable statements are introduced.
Issue Number:
Fixes #2594
Snapshots/Videos:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests