feat: update fmt scripts to use black with jupyter ext #7342
Conversation
|
The tests were failing because I passed the 🐕 ❯ pytest -p no:warnings dev_tools/bash_scripts_test.py::test_incremental_format_branch_selection
=============================================================================== test session starts ===============================================================================
platform darwin -- Python 3.11.11, pytest-8.3.5, pluggy-1.5.0
rootdir: /Users/sauravmaheshkar/dev/Cirq
configfile: pyproject.toml
collected 1 item
dev_tools/bash_scripts_test.py . [100%]
================================================================================ 1 passed in 3.83s ================================================================================ |
SauravMaheshkar
force-pushed
the
saurav/black-jupyter-script
branch
from
a49cdcd to
74b272e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7342 +/- ##
==========================================
- Coverage 98.67% 98.67% -0.01%
==========================================
Files 1112 1112
Lines 97038 97038
==========================================
- Hits 95757 95755 -2
- Misses 1281 1283 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -87,14 +87,23 @@ if (( only_changed == 1 )); then | |||
| else | |||
There was a problem hiding this comment.
The if-branch above (lines 84-86) does not include .ipynb files. Please correct.
| for f in "${format_files[@]}"; do | ||
| if [[ "${f}" == *.ipynb ]]; then | ||
| format_ipynb_files+=("${f}") | ||
| else | ||
| format_py_files+=("${f}") | ||
| fi | ||
| done |
There was a problem hiding this comment.
There is no need to separate .ipynb and .py files into two arrays.
Black can detect the file type of its arguments, just run it without the --ipynb option.
| format_py_files+=("${f}") | ||
| fi | ||
| done | ||
|
|
There was a problem hiding this comment.
Lines 110-112 - the isort_files array would include .ipynb files if they were in format_files, but it should only contain .py files. Please adjust the condition on line 110.
| black "${args[@]}" "${format_files[@]}" | ||
| BLACKSTATUS=$? | ||
| BLACKSTATUS=0 | ||
| if (( "${#format_py_files[@]}" )); then | ||
| black "${args[@]}" "${format_py_files[@]}" | ||
| BLACKSTATUS=$? | ||
| fi | ||
| if (( "${#format_ipynb_files[@]}" )); then | ||
| black "${args[@]}" --ipynb "${format_ipynb_files[@]}" | ||
| BLACKSTATUS=$((BLACKSTATUS || $?)) | ||
| fi |
There was a problem hiding this comment.
Please revert back. It works OK if black is run without the --ipynb option.
Following up from #7256
This is the 1st PR updating the scripts and the requirements. Follow up PR with the actual changes will follow.
CC: @pavoljuhas
Output