You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "asukaminato0721 (via GitHub)" <gi...@apache.org> on 2024/02/03 16:23:04 UTC
[PR] Add ruff [opendal]
asukaminato0721 opened a new pull request, #4135:
URL: https://github.com/apache/opendal/pull/4135
try fix #3468
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #4135:
URL: https://github.com/apache/opendal/pull/4135
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #4135:
URL: https://github.com/apache/opendal/pull/4135#issuecomment-1925401873
Python CI not running for:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "asukaminato0721 (via GitHub)" <gi...@apache.org>.
asukaminato0721 commented on PR #4135:
URL: https://github.com/apache/opendal/pull/4135#issuecomment-1925422349
there are some more lint rules
- isort https://github.com/astral-sh/ruff/issues/465#issuecomment-1783684428
- format (still under discussion, so leave it be there) https://github.com/astral-sh/ruff/issues/8232
could be done in further pr, leave a note here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "asukaminato0721 (via GitHub)" <gi...@apache.org>.
asukaminato0721 commented on PR #4135:
URL: https://github.com/apache/opendal/pull/4135#issuecomment-1925410663
now it break the ci, which means works :)
```
benchmark/async_origin_s3_benchmark_with_gevent.py:22:1: E402 Module level import not at top of file
benchmark/async_origin_s3_benchmark_with_gevent.py:26:1: E402 Module level import not at top of file
benchmark/async_origin_s3_benchmark_with_gevent.py:28:1: E402 Module level import not at top of file
benchmark/async_origin_s3_benchmark_with_gevent.py:29:1: E402 Module level import not at top of file
benchmark/async_origin_s3_benchmark_with_gevent.py:30:1: E402 Module level import not at top of file
benchmark/async_origin_s3_benchmark_with_gevent.py:31:1: E402 Module level import not at top of file
python/opendal/__init__.py:18:1: F403 `from ._opendal import *` used; unable to detect undefined names
python/opendal/__init__.py:20:11: F405 `_opendal` may be undefined, or defined from star imports
python/opendal/__init__.py:21:11: F405 `_opendal` may be undefined, or defined from star imports
```
I guess we could ignore these first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4135:
URL: https://github.com/apache/opendal/pull/4135#discussion_r1477102501
##########
.github/workflows/bindings_python.yml:
##########
@@ -40,6 +40,10 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
+ - name: Lint
+ with:
Review Comment:
We can use
```yaml
working-directory: "bindings/python"
run: |
pip install ruff
ruff check .
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4135:
URL: https://github.com/apache/opendal/pull/4135#discussion_r1477103371
##########
.github/workflows/bindings_python.yml:
##########
@@ -40,6 +40,10 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
+ with:
+ working-directory: "bindings/python"
+ run: |
Review Comment:
Hi, this section seems to added into `actions/checkout@v4` wrongly.
We need to add a new step here like:
```yaml
- name: Ruff Check
working-directory: "bindings/python"
run: |
pip install ruff
ruff check .
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ruff [opendal]
Posted by "asukaminato0721 (via GitHub)" <gi...@apache.org>.
asukaminato0721 commented on code in PR #4135:
URL: https://github.com/apache/opendal/pull/4135#discussion_r1477090916
##########
.github/actions/behavior_test_binding_python/action.yaml:
##########
@@ -43,6 +43,7 @@ runs:
working-directory: bindings/python
run: |
source venv/bin/activate
+ ruff check .
Review Comment:
ok, I find that `.github/workflows/bindings_python.yml` , is digging into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4135:
URL: https://github.com/apache/opendal/pull/4135#discussion_r1477096059
##########
.github/workflows/bindings_python.yml:
##########
@@ -82,6 +86,10 @@ jobs:
- uses: actions/checkout@v4
- name: Setup Rust toolchain
uses: ./.github/actions/setup
+ - name: Ruff Check
+ uses: jpetrucciani/ruff-check@0.1.0
Review Comment:
Install ruff should be simple by calling pip install. Please don't use extra actions so that we don't need to review and maintain them.
##########
.github/workflows/bindings_python.yml:
##########
@@ -60,6 +60,10 @@ jobs:
- uses: actions/checkout@v4
- name: Setup Rust toolchain
uses: ./.github/actions/setup
+ - name: Ruff Check
Review Comment:
The check results are the same on all platforms, so we don't need to check everywhere. How about checking it in the sdist job?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ruff [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4135:
URL: https://github.com/apache/opendal/pull/4135#discussion_r1477090013
##########
.github/actions/behavior_test_binding_python/action.yaml:
##########
@@ -43,6 +43,7 @@ runs:
working-directory: bindings/python
run: |
source venv/bin/activate
+ ruff check .
Review Comment:
We don't need to run ruff check for every behavior test. We can do it in binding_python CI instead, along with build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): add ruff as linter [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #4135:
URL: https://github.com/apache/opendal/pull/4135#issuecomment-1925410754
Thanks for your effort! Now our ruff check is running.
Lint like the following could be ignored:
```shell
python/opendal/__init__.py:18:1: F403 `from ._opendal import *` used; unable to detect undefined names
python/opendal/__init__.py:20:11: F405 `_opendal` may be undefined, or defined from star imports
python/opendal/__init__.py:21:11: F405 `_opendal` may be undefined, or defined from star imports
```
`_opendal` is from our rust binding that not in current context.
And the error like
```
benchmark/async_origin_s3_benchmark_with_gevent.py:31:1: E402 Module level import not at top of file
```
Could be fixed easily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org