Centralize Python linter/formatter requirements #8854
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a significant restructuring of development requirements and workflow configurations. The changes primarily involve consolidating development dependencies into a new Changes
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
CodeRabbit Configuration File (
|
SpecLad
force-pushed
the
central-dev-reqs
branch
2 times, most recently
from
2a435b5 to
93b0ee8
Compare
These are currently duplicated all over the codebase, often with mismatched constraints. Put them all in one requirements file instead. Fix a few minor issues while I'm at it: * Constrain black to the current major version to avoid New Year surprises (black can change styles between major versions). Constrain isort too, just in case. * Remove usages of `egrep`, which is deprecated (and unnecessary here).
SpecLad
force-pushed
the
central-dev-reqs
branch
from
93b0ee8 to
60b1c93
Compare
![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 (2)
dev/requirements.txt (1)
1-5: Consider standardizing version constraint approachThe version constraints are inconsistent:
- Formatters use major version constraints (
24.*,5.*)- Linters use exact versions (
2.14.5,2.5.3,0.7)Consider using similar constraint patterns for better maintainability:
black==24.* isort==5.* -pylint-django==2.5.3 -pylint-plugin-utils==0.7 -pylint==2.14.5 +pylint-django~=2.5.0 +pylint-plugin-utils~=0.7.0 +pylint~=2.14.0This allows for patch updates while maintaining compatibility.
.github/workflows/black.yml (1)
11-11: Add error handling for requirement extractionThe grep command might fail if the requirement format changes. Consider adding error handling:
- pipx install $(grep "^black" ./dev/requirements.txt) + BLACK_REQ=$(grep "^black==" ./dev/requirements.txt) + if [ -z "$BLACK_REQ" ]; then + echo "Error: Could not find black requirement" + exit 1 + fi + pipx install $BLACK_REQ
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cvat-sdk/gen/requirements.txtis excluded by!**/gen/**
📒 Files selected for processing (10)
.github/workflows/black.yml(1 hunks).github/workflows/isort.yml(1 hunks).github/workflows/pylint.yml(1 hunks)cvat-cli/requirements/development.txt(0 hunks)cvat/requirements/all.txt(0 hunks)cvat/requirements/development.in(0 hunks)cvat/requirements/development.txt(1 hunks)cvat/requirements/testing.txt(0 hunks)dev/requirements.txt(1 hunks)site/requirements.txt(0 hunks)
💤 Files with no reviewable changes (5)
- site/requirements.txt
- cvat-cli/requirements/development.txt
- cvat/requirements/testing.txt
- cvat/requirements/all.txt
- cvat/requirements/development.in
✅ Files skipped from review due to trivial changes (1)
- cvat/requirements/development.txt
🔇 Additional comments (3)
.github/workflows/pylint.yml (1)
22-26: 🛠️ Refactor suggestion
Improve dependency installation robustness
The current implementation has several potential issues:
- No error handling for requirement extraction
- Potential version conflicts between pylint plugins and Django
Consider this more robust approach:
- pipx install $(grep "^pylint==" ./dev/requirements.txt)
-
- pipx inject pylint \
- $(grep "^pylint-.\+==" ./dev/requirements.txt) \
- $(grep "^django==" ./cvat/requirements/base.txt)
+ # Extract requirements
+ PYLINT_REQ=$(grep "^pylint==" ./dev/requirements.txt)
+ PYLINT_PLUGINS=$(grep "^pylint-.\+==" ./dev/requirements.txt)
+ DJANGO_REQ=$(grep "^django==" ./cvat/requirements/base.txt)
+
+ # Validate requirements were found
+ if [ -z "$PYLINT_REQ" ] || [ -z "$PYLINT_PLUGINS" ] || [ -z "$DJANGO_REQ" ]; then
+ echo "Error: Could not find all required packages"
+ exit 1
+ fi
+
+ # Install pylint and inject dependencies
+ pipx install $PYLINT_REQ
+ pipx inject pylint $PYLINT_PLUGINS $DJANGO_REQLet's verify Django version compatibility:
✅ Verification successful
Refactoring suggestion is valid but version compatibility is not a concern
The original review comment's refactoring suggestion to improve error handling is valuable, but the concern about version conflicts between Django and pylint-django is unfounded:
- pylint-django 2.5.3 doesn't enforce strict Django version requirements (only optional with 'with_django' extra)
- Django 4.2.17 is a stable release and pylint-django is known to work with Django 4.x
- The current setup in the repository has been working with these exact versions
The suggested error handling improvements remain beneficial for robustness:
- Validating package existence in requirements files
- Using intermediate variables for better debugging
- Proper error reporting and exit codes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Django version constraints in both files
echo "Django version in base.txt:"
grep "^django==" ./cvat/requirements/base.txt
echo "Django version supported by pylint-django:"
grep "^pylint-django==" ./dev/requirements.txt
Length of output: 292
Script:
#!/bin/bash
# Check pylint-django package metadata for Django version compatibility
echo "Checking pylint-django package metadata:"
curl -s https://pypi.org/pypi/pylint-django/2.5.3/json | jq -r '.info.requires_dist[] | select(contains("Django"))'
# Check if there are any Django version constraints in pylint core
echo -e "\nChecking pylint version:"
grep "^pylint==" ./dev/requirements.txt
echo -e "\nChecking other pylint plugins that might have Django dependencies:"
grep "^pylint-" ./dev/requirements.txt
Length of output: 598
.github/workflows/isort.yml (2)
28-28: LGTM! Changes align with PR objectives.
The modification successfully:
- Centralizes the isort requirement in the new
dev/requirements.txt - Removes the deprecated
egrepusage
28-28: Consider using absolute path for requirements file.
To make the script more robust against working directory changes, consider using $GITHUB_WORKSPACE:
- pipx install $(grep "^isort" ./dev/requirements.txt)
+ pipx install $(grep "^isort" "${GITHUB_WORKSPACE}/dev/requirements.txt")Let's verify the requirements file setup:
|
This might seem a bit silly, but adding a spell-checker to CI would also be nice (just as already recommended here), this will enforce correct and unified English spelling in all qualifiers across the codebase. I had this funny thing where I'm searching keywords in the codebase and can't find anything only to later find out that they had been misspelled. Misspellings vary by each author so this might become an issue, too |
Alright, updated. |
|
|
@archibald1418 Yeah, it would be nice, but that's a big task in its own right. Incidentally, if you do find misspellings/typos, feel free to submit a PR to fix them. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8854 +/- ##
===========================================
- Coverage 73.88% 73.87% -0.01%
===========================================
Files 408 408
Lines 44086 44104 +18
Branches 3986 3986
===========================================
+ Hits 32575 32584 +9
- Misses 11511 11520 +9
|
Motivation and context
These are currently duplicated all over the codebase, often with mismatched constraints. Put them all in one requirements file instead.
Fix a few minor issues while I'm at it:
Constrain black to the current major version to avoid New Year surprises (black can change styles between major versions). Constrain isort too, just in case.
Remove usages of
egrep, which is deprecated (and unnecessary here).How has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
black,isort, andpylint, to enhance code quality and consistency.Bug Fixes
Documentation
Chores