Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ruff tool #773

Merged
merged 11 commits into from Dec 26, 2022
Merged

Add ruff tool #773

merged 11 commits into from Dec 26, 2022

Conversation

marscher
Copy link
Contributor

@marscher marscher commented Nov 15, 2022

I'd like an suggestion what a useful test would look like. I believe this tools output could likely change in the future, so the usefulness of strict output checking right now seems overkill.
You have to decide.

Fixes #772

@marscher
Copy link
Contributor Author

@MarcoGorelli I think this is ready to be reviewed!

Copy link
Collaborator

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Couple more requests:

  • can we add ruff to

nbQA/setup.cfg

Line 55 in 19d0e60

pyupgrade

?

  • can we add it a ruff example to

https://github.com/nbQA-dev/nbQA/blob/main/docs/examples.rst

?

  • let's also add a link here

nbQA/README.md

Line 135 in 19d0e60

See [command-line examples](https://nbqa.readthedocs.io/en/latest/examples.html) for examples involving [doctest](https://docs.python.org/3/library/doctest.html), [flake8](https://flake8.pycqa.org/en/latest/), [mypy](http://mypy-lang.org/), [pylint](https://github.com/PyCQA/pylint), [autopep8](https://github.com/hhatto/autopep8), [pydocstyle](http://www.pydocstyle.org/en/stable/), and [yapf](https://github.com/google/yapf).

tests/tools/test_ruff_works.py Outdated Show resolved Hide resolved
@marscher
Copy link
Contributor Author

Thanks for the review. I've added the requested changes.

@MarcoGorelli
Copy link
Collaborator

Thanks! I'll try this out later but it should be good to go

@marscher
Copy link
Contributor Author

marscher commented Nov 15, 2022

I just tested the hook locally and I need to pass the "verbose" flag to pre-commit so the errors show up. Also it does not fail, even if the passed notebook contains errors. What am I missing?

Ruff returns a code != 0, in case of an error.

@MarcoGorelli
Copy link
Collaborator

Hey - sorry, just getting back to this now

Also it does not fail, even if the passed notebook contains errors

hmm this doesn't look quite right - could you show an example please?

Also, looks like I need to fix compat with flake8 6.0.0 to get CI to pass, I'll do that separately

@MarcoGorelli
Copy link
Collaborator

Can't reproduce this BTW:

(.venv) marcogorelli@DESKTOP-U8OKFP3:~/nbQA-dev$ nbqa ruff tests/data/notebook_for_testing.ipynb 
Found 7 error(s).
tests/data/notebook_for_testing.ipynb:cell_1:1:8: F401 `os` imported but unused
tests/data/notebook_for_testing.ipynb:cell_1:3:8: F401 `glob` imported but unused
tests/data/notebook_for_testing.ipynb:cell_1:5:8: F401 `nbqa` imported but unused
tests/data/notebook_for_testing.ipynb:cell_4:1:1: E402 Module level import not at top of file
tests/data/notebook_for_testing.ipynb:cell_4:1:20: F401 `random.randint` imported but unused
tests/data/notebook_for_testing.ipynb:cell_5:1:1: E402 Module level import not at top of file
tests/data/notebook_for_testing.ipynb:cell_5:2:1: E402 Module level import not at top of file
4 potentially fixable with the --fix option.
(.venv) marcogorelli@DESKTOP-U8OKFP3:~/nbQA-dev$ echo $?
1

@MarcoGorelli
Copy link
Collaborator

I just tested the hook locally and I need to pass the "verbose" flag to pre-commit so the errors show up. Also it does not fail, even if the passed notebook contains errors. What am I missing?

Ruff returns a code != 0, in case of an error.

Still can't reproduce this:

(.venv) marcogorelli@DESKTOP-U8OKFP3:~/tmp$ pre-commit run nbqa-ruff -a
nbqa-ruff................................................................Failed
- hook id: nbqa-ruff
- exit code: 1

notebook_for_testing.ipynb:cell_1:1:8: F401 `os` imported but unused
notebook_for_testing.ipynb:cell_1:3:8: F401 `glob` imported but unused
notebook_for_testing.ipynb:cell_1:5:8: F401 `nbqa` imported but unused
notebook_for_testing.ipynb:cell_4:1:1: E402 Module level import not at top of file
notebook_for_testing.ipynb:cell_4:1:20: F401 `random.randint` imported but unused
notebook_for_testing.ipynb:cell_5:1:1: E402 Module level import not at top of file
notebook_for_testing.ipynb:cell_5:2:1: E402 Module level import not at top of file
Found 7 error(s).
4 potentially fixable with the --fix option.

Let's ship it then, if any bug is reported we can address later

@MarcoGorelli MarcoGorelli merged commit 0b44015 into nbQA-dev:main Dec 26, 2022
@MarcoGorelli
Copy link
Collaborator

@all-contributors please add @marscher for features

@allcontributors
Copy link
Contributor

@MarcoGorelli

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@MarcoGorelli
Copy link
Collaborator

@all-contributors please add @marscher for code

@allcontributors
Copy link
Contributor

@MarcoGorelli

I've put up a pull request to add @marscher! 🎉

@charliermarsh
Copy link

Apologies for the spam, but this is really cool! Thank you for the integration!

@MarcoGorelli
Copy link
Collaborator

Apologies for the spam, but this is really cool! Thank you for the integration!

thanks!

BTW are you signed up to github sponsors? I'd love to sponsor ruff

@charliermarsh
Copy link

@MarcoGorelli - Thank you, that's super kind! I'm not signed up yet but I am considering it. I will let you know if I turn it on :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Ruff
3 participants