Python icon indicating copy to clipboard operation
Python copied to clipboard

Flake8 plugins in pre-commit

Open CaedenPH opened this issue 2 years ago • 5 comments

Feature description

Certain flake8 plugins increase code quality. Two of these plugins have been implemented so far, pep8-naming and flake8-builtins. What other plugins should be implemented?

Would you like to work on this feature?

  • [X] Yes, I want to work on this feature!

CaedenPH avatar Oct 13 '22 15:10 CaedenPH

@cclauss @dhruvmanila

CaedenPH avatar Oct 13 '22 15:10 CaedenPH

Maybe docstrings? https://github.com/PyCQA?q=flake8- But I am sure a ton of existing files do not have docstrings.

Also like flake8-2020, flake8-bugbear, flake8-comprehensions, flake8-return, flake8-simplify but... we need to be careful not to go overboard and make contributors too angry to fix all the nits.

cclauss avatar Oct 13 '22 15:10 cclauss

Personally I would go for flake8-bugbear, docstrings would be a hell of a jump

CaedenPH avatar Oct 13 '22 15:10 CaedenPH

Simplify and comprehensions come up with some quite useful suggestions.

Return is helpful for beginners but they have a few errors that I do not agree with.

cclauss avatar Oct 13 '22 15:10 cclauss

Can I make a pull request implementing flake8-bugbear?

CaedenPH avatar Oct 13 '22 15:10 CaedenPH

Plugins that are useful for me are:

My personal list is quite huge, but I find it too strict for the project with many contributors. https://github.com/Cjkjvfnby/project_template/blob/master/%7B%7Bcookiecutter.folder_name%7D%7D/.pre-commit-config.yaml

The thing I learned is that things are constantly broken because of library updates, so I prefer to use fixed versions. Especially because I use pre-commit as a GitHub action.

If you wold like, I could contribute adding this plugins.

Cjkjvfnby avatar Oct 27 '22 20:10 Cjkjvfnby

To run flake8 with plugins you need to install them first.
This line in the contribution guide don't mention this.

Since flake8 is already included in pre-commit run --all-files ... , this string might be just removed.

All submissions will need to pass the test flake8 . --ignore=E203,W503 --max-line-length=88 before they will be accepted so if possible, try this test locally on your Python file(s) before submitting your pull request.

Cjkjvfnby avatar Oct 27 '22 20:10 Cjkjvfnby

For this repo, I would say no to both eradicate and pytest style.

  • Eradicate -- This is not a production code repo -- instead, it is a center for learning and sharing so commented code might be quite acceptable and removing comments without human intervention sounds scary to me.
  • Pytest -- We focus a lot on doctest and have very few pytests. Of those few that we do have, we do not want to set the bar for contribution to be much higher than pass/fail.

cclauss avatar Oct 27 '22 20:10 cclauss

Most of our contributors are first-time-contributors and almost none of them have pre-commit installed and running locally which is why we run pre-commit.ci but a pull request to fix README.md would be welcome.

cclauss avatar Oct 27 '22 20:10 cclauss

flake8-use-pathlib sounds awesome (although I have never seen it in action).

cclauss avatar Oct 27 '22 20:10 cclauss

Maybe docstrings? https://github.com/PyCQA?q=flake8- But I am sure a ton of existing files do not have docstrings.

Flake is pretty flexible with ignoring files with specific errors.
You could suppress specific errors for files in the flake config. It looks a bit bulky but allows you to avoid touching the code. https://flake8.pycqa.org/en/latest/user/options.html#cmdoption-flake8-per-file-ignores

I use this plugin for my code and exclude test files from it.

per-file-ignores =
    # No docs and annotation required for tests
    tests/*.py:    D10

Cjkjvfnby avatar Oct 27 '22 20:10 Cjkjvfnby

A ton means a ton -- take a scroll.

cclauss avatar Oct 27 '22 20:10 cclauss

flake8-use-pathlib sounds awesome (although I have never seen it in action).

I have some concerns about it. This code looks super unusual to me.

with Path(filename).open() as f:

A ton means a ton -- take a scroll.

I could extract errors from the flake8 output and just build a flake8 setting from it. So the new contributions will have docstrings.

Cjkjvfnby avatar Oct 27 '22 20:10 Cjkjvfnby

OK. I will try out use-pathlib and see for myself.

If you want to do a PR on docstrings, that would be reviewed. Thx.

cclauss avatar Oct 27 '22 20:10 cclauss

Sample PR on how the code should look to conform the flake8-docstrings. https://github.com/TheAlgorithms/Python/pull/7821

It would be nice to understand the check that we plan to keep and which one we could ignore.

Fixing all of them sounds boring. We could either ignore them or keep checking only for new files. Maybe pre-commit.ci could just ignore existing errors and just force to fix them a person who is editing the files.

Cjkjvfnby avatar Oct 28 '22 21:10 Cjkjvfnby

I'd say it is closed a bit too earlier. I haven't submitted the PR with annotation-docstring yet. I am planning to do it today in the evening, so no need to reopen it.

I have learned a lot of cool things from yours pre-commit hooks, things like yesqa are exactly what I need in my projects.

Cjkjvfnby avatar Oct 30 '22 09:10 Cjkjvfnby

I need to try to review the 150+ current open PRs so please hold off on adding more PRs until Tuesday, Nov 1st.

cclauss avatar Oct 30 '22 09:10 cclauss

I have a free evening today, could I help with the review?

Cjkjvfnby avatar Oct 30 '22 13:10 Cjkjvfnby