Update format-incremental script to format notebooks #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.68% 98.68% -0.01%
==========================================
Files 1112 1112
Lines 97641 97641
==========================================
- Hits 96360 96357 -3
- Misses 1281 1284 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
check/format-incremental
Outdated
| 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.
check/format-incremental
Outdated
| 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.
check/format-incremental
Outdated
| 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.
`black` accepts both python sources and notebooks as its arguments. Also check formatting of notebooks that changed w/r to main.
pavoljuhas
changed the title
pavoljuhas
changed the title
| fi | ||
|
|
||
| if (( ${#format_files[@]} == 0 )); then | ||
| echo -e "\033[32mNo files to format.\033[0m" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Exclude __init__.py files from isort targets | ||
| # Exclude notebooks and __init__.py files from isort targets |
There was a problem hiding this comment.
This says notebooks, but the code below it tests against .py files. I'm probably missing something, but flagging it anyway in case it's a genuine discrepancy.
This is the 1st PR updating the scripts and the requirements.
Follow up PR with the actual changes will follow.
Partially implements #3976
Output