Simplify the code formatting script #8910
Conversation
After cvat-ai#8611 and cvat-ai#8866, it's no longer necessary to apply the formatters to each module separately. So don't! In addition, update the black exclusion list in `pyproject.toml` to only contain files that were _not_ listed in `format_python_code.sh`. This way, formatting will automatically be enforced for all new files. For a few of the files reformatting only creates a small diff, so don't even bother excluding them.
SpecLad
requested review from
azhavoro,
Marishka17 and
nmanovic
as code owners
WalkthroughThis pull request involves a series of code formatting and standardization changes across multiple Python files in the CVAT project. The modifications primarily focus on improving code readability by restructuring conditional expressions, updating string quotation styles, and enhancing the Python code formatting script. The changes are mostly cosmetic, targeting the consistency and clarity of the codebase without altering the underlying functionality of the code. 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 (
|
|
![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 UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cvat/apps/engine/management/commands/syncperiodicjobs.py(4 hunks)cvat/apps/engine/rules/tests/generators/annotationguides_test.gen.rego.py(2 hunks)cvat/apps/engine/rules/tests/generators/cloudstorages_test.gen.rego.py(1 hunks)cvat/apps/engine/rules/tests/generators/comments_test.gen.rego.py(1 hunks)cvat/apps/engine/rules/tests/generators/issues_test.gen.rego.py(1 hunks)cvat/apps/engine/rules/tests/generators/jobs_test.gen.rego.py(1 hunks)cvat/apps/engine/rules/tests/generators/projects_test.gen.rego.py(1 hunks)cvat/apps/engine/rules/tests/generators/server_test.gen.rego.py(1 hunks)cvat/apps/engine/rules/tests/generators/tasks_test.gen.rego.py(1 hunks)cvat/apps/engine/rules/tests/generators/users_test.gen.rego.py(2 hunks)dev/format_python_code.sh(1 hunks)pyproject.toml(1 hunks)site/content/en/docs/contributing/coding-style.md(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- cvat/apps/engine/rules/tests/generators/jobs_test.gen.rego.py
- cvat/apps/engine/rules/tests/generators/cloudstorages_test.gen.rego.py
- cvat/apps/engine/rules/tests/generators/comments_test.gen.rego.py
- cvat/apps/engine/rules/tests/generators/tasks_test.gen.rego.py
- cvat/apps/engine/rules/tests/generators/server_test.gen.rego.py
- cvat/apps/engine/management/commands/syncperiodicjobs.py
- cvat/apps/engine/rules/tests/generators/annotationguides_test.gen.rego.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: testing
- GitHub Check: generate_github_pages
- GitHub Check: Analyze (python)
- GitHub Check: Linter
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
cvat/apps/engine/rules/tests/generators/users_test.gen.rego.py (2)
Line range hint
56-74: LGTM! Well-structured rule evaluation logic.The implementation is clean and efficient, using filter operations to progressively narrow down the applicable rules.
85-93: LGTM! Improved readability with multi-line formatting.The conditional expression is now more readable while maintaining the same logic.
cvat/apps/engine/rules/tests/generators/projects_test.gen.rego.py (1)
115-123: LGTM! Consistent formatting improvement.The conditional expression formatting matches the style used in other files, improving readability.
cvat/apps/engine/rules/tests/generators/issues_test.gen.rego.py (1)
129-137: LGTM! Consistent formatting improvement.The conditional expression formatting matches the style used in other files, improving readability.
dev/format_python_code.sh (1)
18-19: LGTM! Simplified approach to code formatting.The change to run formatters on the current directory (
.) is a good simplification that ensures all files are covered without maintaining a list of paths. This aligns well with the PR objectives.Let's verify the impact of this change:
✅ Verification successful
Simplified formatting approach is safe to use
The change to use
.is safe because:
- Black's exclusion patterns in pyproject.toml are path-based and will continue to work correctly
- The exclusions use absolute paths from the repository root ('^/'), ensuring consistent behavior
- The excluded paths are marked as temporary ("TODO: get rid of these"), so this change aligns with the project's direction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check which Python files would be affected by the new formatting approach # that weren't covered before # First, list all Python files echo "Python files in the repository:" fd '\.py$' # Then, check which ones are excluded by black echo -e "\nFiles excluded by black:" ${PYTHON:-python3} -m black --extend-exclude "$(grep -A 100 '^extend-exclude = """' pyproject.toml | grep -B 100 '^"""' | tail -n +2)" --verbose . 2>&1 | grep "excluded"Length of output: 23996
Script:
#!/bin/bash # First, let's get the black configuration and see what files would be formatted echo "Files that would be formatted by black:" python3 -m black --verbose --check . 2>&1 | grep "would reformat" || true echo -e "\nFiles that would be skipped by black:" python3 -m black --verbose --check . 2>&1 | grep "excluded" || true # Also get the content of pyproject.toml to verify the exclusion patterns echo -e "\nBlack configuration from pyproject.toml:" cat pyproject.toml | grep -A 50 "\[tool.black\]" | grep -B 50 "^$" || trueLength of output: 484
Script:
#!/bin/bash # Find pyproject.toml first echo "Looking for pyproject.toml:" fd pyproject.toml # Show full black output echo -e "\nFull black check output:" python3 -m black --check --verbose . 2>&1 || true # If pyproject.toml exists, show its content echo -e "\nContent of pyproject.toml (if found):" fd pyproject.toml -x cat {}Length of output: 3346
site/content/en/docs/contributing/coding-style.md (1)
14-14: LGTM! Clear and concise documentation update.The documentation change effectively communicates the new simplified approach to code formatting.
pyproject.toml (1)
Line range hint
16-16: Verify Python version compatibility.The black configuration targets Python 3.9. Please verify this aligns with the project's minimum supported Python version.
Let's check the project's Python version requirements:
✅ Verification successful
Black's Python 3.9 target version is compatible with project requirements ✓
The project requires Python >= 3.9 as specified in cvat-cli's setup.py, which aligns perfectly with Black's Python 3.9 target version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Python version requirements in the project echo "Setup.py Python version requirement:" fd 'setup\.py$' -x grep -l "python_requires" {} \; -x cat {} echo -e "\nDocumented Python version requirement:" fd -e md -e rst -x grep -l "python" {} \; -x grep -l "version" {} \; -x cat {}Length of output: 68486
| ${BLACK} . | ||
| ${ISORT} --resolve-all-configs . |
There was a problem hiding this comment.
I'm a little bit worried that these commands now don't have any reference to CVAT specifically, so starting them from a directory outside of CVAT repo may have unexpected consequences. This doesn't look like a big problem, but maybe add a check that the current directory is a path inside a CVAT repo?
Motivation and context
After #8611 and #8866, it's no longer necessary to apply the formatters to each module separately. So don't!
In addition, update the black exclusion list in
pyproject.tomlto only contain files that were not listed informat_python_code.sh. This way, formatting will automatically be enforced for all new files.For a few of the files reformatting only creates a small diff, so don't even bother excluding them - reformat them instead.
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
Chores
isortandblacktoolsStyle