Enhance CI formatting checks for Python files #600
Conversation
There was a problem hiding this comment.
Use git rebase -i to squash and rework git commit messages.
Enforce the rules mentioned in https://cbea.ms/git-commit/ .
Lzzz666
force-pushed
the
python-formatter
branch
from
4cd6137 to
3d9ea88
Compare
There was a problem hiding this comment.
Add a description about Python code formatting in the following section:
Line 34 in 88ddb73
CONTRIBUTING.md
Outdated
| @@ -48,9 +48,10 @@ However, participation requires adherence to fundamental ground rules: | |||
| Software requirement: | |||
| * [clang-format](https://clang.llvm.org/docs/ClangFormat.html) version 18 or later. | |||
| * [shfmt](https://github.com/mvdan/sh). | |||
| * [black](https://github.com/psf/black) for Python code formatting. | |||
There was a problem hiding this comment.
Simply mention black is informative.
CONTRIBUTING.md
Outdated
| This repository consistently contains an up-to-date `.clang-format` file with rules that match the explained ones and uses shell script formatting supported by `shfmt`. | ||
| For maintaining a uniform coding style, execute the command `clang-format -i *.{c,h}` and `shfmt -w $(find . -type f -name "*.sh")`. | ||
| This repository consistently contains an up-to-date `.clang-format` file with rules that match the explained ones, uses shell script formatting supported by `shfmt`, and Python code formatting with `black`. | ||
| For maintaining a uniform coding style, execute the command `clang-format -i *.{c,h}`, `shfmt -w $(find . -type f -name "*.sh")`, and `black .` for Python files. |
There was a problem hiding this comment.
Refine the sentence to emphasize the goal of each command for different target programming languages.
.ci/check-format.sh
Outdated
| PY_MISMATCH_FILE_CNT=0 | ||
| if [ -n "${PY_SOURCES}" ]; then | ||
| PY_MISMATCH_OUTPUT=$(black --check ${PY_SOURCES} 2>&1) | ||
| PY_MISMATCH_FILE_CNT=$(echo "${PY_MISMATCH_OUTPUT}" | grep -c "^would reformat ") |
There was a problem hiding this comment.
Combine two statements into single for simplicity:
PY_MISMATCH_FILE_CNT=$(echo "$(black --check ${PY_SOURCES} 2>&1)" | grep -c "^would reformat ")
|
@ChinYikMing I’ve pushed the updates, please check! |
CONTRIBUTING.md
Outdated
| Python scripts must be clean, consistent, and adhere to modern Python best practices. The following formatting rules are enforced project-wide using `black`: | ||
|
|
||
| * Use 4 spaces for indentation (never tabs). | ||
| * Limit lines to 88 characters maximum. |
There was a problem hiding this comment.
Since the CONTRIBUTING.md specifies an 80 characters per line limit for C code, I noticed that 88 characters are used here for Python. I'm curious - how was the number 88 chosen? Should we consider aligning the limit across languages?
There was a problem hiding this comment.
I agree with your suggestion—having the same limit across languages makes sense. I initially chose 88 characters simply because that’s Black’s default.
|
I noticed that your commit message mentions several different changes in a single patch. Would it be better to split them into separate patches? For example, one patch could fix the Python code style, and another could add the CI check - instead of combining them all in one. |
Lzzz666
force-pushed
the
python-formatter
branch
2 times, most recently
from
cc66024 to
5108e0d
Compare
Lzzz666
force-pushed
the
python-formatter
branch
from
5108e0d to
7333c05
Compare
Lzzz666
force-pushed
the
python-formatter
branch
from
7333c05 to
602bee8
Compare
pyproject.toml
Outdated
| @@ -0,0 +1,11 @@ | |||
| [tool.black] | |||
| line-length = 80 | |||
| target-version = ['py37', 'py38', 'py39', 'py310', 'py311'] | |||
There was a problem hiding this comment.
Why not including py312? Just for curiosity, any reason for that (e.g., parser compatibility)?
There was a problem hiding this comment.
I noticed that using Black v24.8.0 will issue an error when used with Python 3.12.5, due to an upstream memory safety issue. If we decide to support py312, should we also include py313 ? Additionally, should we pin Black to the latest v25.1.0 when installing? Since Black v24.10.0, it immediately errors in a Python 3.12.5 environment, and it requires at least Python 3.9.
pyproject.toml
Outdated
| line-length = 80 | ||
| target-version = ['py37', 'py38', 'py39', 'py310', 'py311'] | ||
| include = '\.pyi?$' | ||
| extend-exclude = ''' |
There was a problem hiding this comment.
Under what circumstances do the .git and build directories generate Python files? If no, we can eliminate the rules in the configuration file for simplicity.
@jserv @visitorckw , can you confirm this?
There was a problem hiding this comment.
Under what circumstances do the
.gitandbuilddirectories generate Python files? If no, we can eliminate the rules in the configuration file for simplicity.
The use of Python scripts is dedicated to code generation at first glance. The formatting of Python code is not really a serious concern since it is used as one-shot tooling.
There was a problem hiding this comment.
Thanks for the suggestion, I’ve removed the extend-exclude rules from Black configuration.
Add Black-based formatting checks for Python files: - Update `.ci/check-format.sh` to include checks for Python files using Black 25.1.0. - Modify `.github/workflows/main.yml` to install Black for formatting. - Add `pyproject.toml` for configuring Black as the Python code formatter. - Enhance `CONTRIBUTING.md` with guidelines for Python formatting.
Lzzz666
force-pushed
the
python-formatter
branch
from
602bee8 to
58e975e
Compare
|
LGTM. |
|
Thank @Lzzz666 for contributing! |
.ci/check-format.shto include checks for Python files using Black..github/workflows/main.ymlto install Python dependencies for formatting..editorconfigto enforce consistent formatting rules.This ensures that both shell scripts and Python files adhere to defined coding standards.
Summary by Bito
This pull request enhances CI formatting checks by integrating support for Python files using the Black formatter. It updates CI scripts and GitHub Actions to include Python-specific checks and dependencies, while refactoring existing Python code for improved readability and maintainability. Additionally, the JIT template generation logic has been refactored for enhanced clarity.