feat: Lint Python with Ruff #4105
Conversation
github-actions
bot
added
engine
dependencies
aglinxinyuan
left a comment
aglinxinyuan
left a comment
There was a problem hiding this comment.
Explain how to use the tool in the PR description.
aglinxinyuan
left a comment
aglinxinyuan
left a comment
There was a problem hiding this comment.
LGTM!
Please let people know they need to pip install ruff
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
Late LGTM. Thanks!
Left some minor comments, it will be good to address them with follow up PRs.
| proto | ||
| )/ | ||
| ''' | ||
| target-version = "py310" |
There was a problem hiding this comment.
I think it is better to always target the highest python version we support, in this case, python 3.12.
@LJX2017 can you change it in a follow up PR?
There was a problem hiding this comment.
According to the official document from Ruff, target-version = "py310" means we support Python >=3.10, including python 3.12 and 3.13. Is this already the correct behavior or should we change it?
There was a problem hiding this comment.
Ok. I had the wrong impression that we should always declare highest version. In this case, let's keep py310. We will need to remember to update it when we drop 3.10 support.
| tx_error "black not found. Install with: pip install black" | ||
| tx_info "Running ruff in amber/src/main/python..." | ||
| if ! command -v ruff >/dev/null 2>&1; then | ||
| tx_error "ruff not found. Install with: pip install ruff" |
There was a problem hiding this comment.
here the instruction should come with the exact version of ruff
3 participants
What changes were proposed in this PR?
Basic Ruff commands:
Under amber/src/main/python
cd amber/src/main/pythonRun Ruff’s formatter in dry mode
ruff format --check .Run Ruff’s formatter
ruff format .Run Ruff’s linter
ruff check .Any related issues, documentation, discussions?
Closes #4078
How was this PR tested?
I created a PR on my own fork to ensure CI is working.
Was this PR authored or co-authored using generative AI tooling?
No