Fixed eslint_disable_check.py (Fixes #3272) #3273
Conversation
WalkthroughThis pull request introduces a new job named "Check-Code-Coverage-Disable" to the GitHub Actions workflow in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/eslint_disable_check.py(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
200-200: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (3)
.github/workflows/scripts/eslint_disable_check.py (3)
41-42: LGTM! Robust regex pattern implementation.The enhanced regex pattern effectively captures all variations of eslint-disable comments including:
- Single-line comments
- Block comments
- Multiline block comments
55-55: LGTM! Consistent error handling.The streamlined error handling provides consistent behavior across all error cases.
71-74: LGTM! Clean and maintainable implementation.The file checking logic is well-structured with:
- Consolidated extension checking
- Consistent error messages
- Clear control flow
Also applies to: 81-84
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3273 +/- ##
=====================================================
+ Coverage 8.49% 89.99% +81.50%
=====================================================
Files 309 330 +21
Lines 7832 8519 +687
Branches 1730 1884 +154
=====================================================
+ Hits 665 7667 +7002
+ Misses 7093 598 -6495
- Partials 74 254 +180
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/scripts/eslint_disable_check.py (2)
71-76: Optimize file checking logic for better readability and performance.The current implementation could be improved by:
- Using early returns to reduce nesting
- Extracting the file checking logic into a separate function
- Optimizing directory traversal
Consider this refactoring:
+def _check_single_file(file_path): + """Check a single file for eslint-disable statements.""" + if not file_path.endswith((".ts", ".tsx")): + return False + + if has_eslint_disable(file_path): + print(f"Error: File {file_path} contains eslint-disable statements.") + return True + return False def check_eslint(files_or_directories): eslint_found = False for item in files_or_directories: if os.path.isfile(item): - if item.endswith((".ts", ".tsx")) and has_eslint_disable(item): - print( - f"Error: File {item} contains eslint-disable statements." - ) - eslint_found = True + eslint_found |= _check_single_file(item) elif os.path.isdir(item): for root, _, files in os.walk(item): if "node_modules" in root: continue for file_name in files: - if file_name.endswith((".ts", ".tsx")): - file_path = os.path.join(root, file_name) - if has_eslint_disable(file_path): - print( - f"Error: File {file_path} contains eslint-disable statements." - ) - eslint_found = True + file_path = os.path.join(root, file_name) + eslint_found |= _check_single_file(file_path) return eslint_foundAlso applies to: 78-89
101-103: Enhance argument parser with validation and detailed help.While the help messages are more concise, consider:
- Adding path validation
- Providing more detailed help with examples
- Adding type hints for better IDE support
Consider these improvements:
+from typing import List, Optional +from pathlib import Path +def validate_path(path: str) -> str: + """Validate if path exists.""" + if not os.path.exists(path): + raise argparse.ArgumentTypeError(f"Path does not exist: {path}") + return path def arg_parser_resolver(): parser = argparse.ArgumentParser( - description="Check TypeScript files for eslint-disable statements." + description="Check TypeScript files for eslint-disable statements.", + epilog="Example: %(prog)s --files src/foo.ts src/bar.ts --directory src/components" ) parser.add_argument( "--files", - type=str, + type=validate_path, nargs="+", default=[], - help="List of files to check for eslint-disable statements.", + help="List of TypeScript files to check (e.g., src/foo.ts).", ) parser.add_argument( "--directory", - type=str, + type=validate_path, nargs="+", default=[os.getcwd()], - help="One or more directories to check for eslint-disable statements.", + help="Directories to recursively check (default: current directory).", )Also applies to: 109-109, 116-116
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/eslint_disable_check.py(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
200-200: shellcheck reported issue in this script: SC2086:info:1:135: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
.github/workflows/pull-request.yml (1)
205-207: LGTM! Efficient conditional execution.The conditional execution ensures the script only runs when TypeScript files are present, improving workflow efficiency.
.github/workflows/scripts/eslint_disable_check.py (1)
41-42: Verify regex pattern coverage and performance.The regex pattern has been enhanced to handle various eslint-disable comment formats. However, consider:
- Adding test cases for edge cases (unusual whitespace, nested comments)
- Performance implications of the pattern's complexity
Let's verify the pattern coverage with this script:
✅ Verification successful
The regex pattern is well-designed and comprehensive
The pattern effectively handles:
- Single-line eslint-disable comments with various formats
- Multi-line block comments with the re.DOTALL flag
- Case-insensitive matching with re.IGNORECASE
- Various whitespace patterns and rule specifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential eslint-disable patterns that might be missed # Create a temporary test file cat << 'EOF' > test_patterns.ts // eslint-disable //eslint-disable // eslint-disable-next-line /*eslint-disable*/ /* eslint-disable */ /* eslint-disable */ /** * eslint-disable */ EOF # Test the script against these patterns python .github/workflows/scripts/eslint_disable_check.py --files test_patterns.ts # Clean up rm test_patterns.tsLength of output: 209
Script:
#!/bin/bash # Create a test file with various eslint-disable patterns cat << 'EOF' > test_patterns.ts // eslint-disable //eslint-disable // eslint-disable-next-line /*eslint-disable*/ /* eslint-disable */ /* eslint-disable */ /** * eslint-disable */ // ESLINT-DISABLE /* ESLINT-DISABLE */ // eslint-disable some-rule, other-rule /* eslint-disable some-rule, other-rule */ EOF # Use ripgrep to test the pattern echo "Testing eslint-disable pattern matches:" rg -i "//\s*eslint-disable(?:-next-line|-line)?[^\n]*|/\*\s*eslint-disable[^\*]*\*/|/\*[\s\S]*?eslint-disable[\s\S]*?\*/" test_patterns.ts # Clean up rm test_patterns.tsLength of output: 441
Script:
#!/bin/bash # Create a test file with various eslint-disable patterns cat << 'EOF' > test_patterns.ts // eslint-disable //eslint-disable // eslint-disable-next-line /*eslint-disable*/ /* eslint-disable */ /* eslint-disable */ /** * eslint-disable */ // ESLINT-DISABLE /* ESLINT-DISABLE */ // eslint-disable some-rule, other-rule /* eslint-disable some-rule, other-rule */ EOF # Use ripgrep with modified pattern to handle multi-line cases echo "Testing with adjusted pattern:" rg -i -U "//\s*eslint-disable(?:-next-line|-line)?[^\n]*|/\*(?:[^*]|\*(?!/))*eslint-disable(?:[^*]|\*(?!/))*\*/" test_patterns.ts # Clean up rm test_patterns.tsLength of output: 595
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
205-207: Consider adding error handling for the Python script.The conditional check ensures the script only runs when TypeScript files are present, but there's no explicit error handling if the Python script fails.
Consider adding error handling:
if: steps.filter-files.outputs.ts_files != '' run: | + set -e # Exit on error python .github/workflows/scripts/eslint_disable_check.py --files ${{ steps.filter-files.outputs.ts_files }} + continue-on-error: false # Ensure the workflow fails if the script fails
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/eslint_disable_check.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/scripts/eslint_disable_check.py
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
200-200: shellcheck reported issue in this script: SC2086:info:1:136: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
|
@adithyanotfound Can the failed test on python code style be fixed? |
Fixed the failed test |
palisadoes
added
the
ignore-sensitive-files-pr
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
Line range hint
207-227: Add error handling and script verification.While the job implementation follows best practices, consider these improvements for robustness:
- name: Run Python script run: | + if [ ! -f .github/workflows/scripts/code_coverage_disable_check.py ]; then + echo "Error: code_coverage_disable_check.py script not found" + exit 1 + fi + chmod +x .github/workflows/scripts/code_coverage_disable_check.py python .github/workflows/scripts/code_coverage_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }} + continue-on-error: false
Line range hint
219-221: Align Python versions across jobs.The workflow uses Python 3.11 in the "Python-Compliance" job but Python 3.9 here. Consider updating to Python 3.11 for consistency:
- name: Set up Python uses: actions/setup-python@v5 with: - python-version: 3.9 + python-version: 3.11
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/eslint_disable_check.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/scripts/eslint_disable_check.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
Line range hint
228-228: LGTM! Dependencies are correctly configured.The addition of "Check-Code-Coverage-Disable" to the needs array ensures proper sequencing of checks before running tests.
palisadoes
merged commit 334f0f7
into
PalisadoesFoundation:develop-postgres
Issue Number:
Fixes #3272
Snapshots/Videos:
Does this PR introduce a breaking change?
No.
Checklist
CodeRabbit AI Review
Test Coverage
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Summary by CodeRabbit
Workflow Updates
Script Improvements