STYLE ignore no-else-* checks, + assorted cleanups

[MarcoGorelli]

@jbrockmendel 's mentioned he's not keen on theno-else-* pylint checks, and to be fair, they are kinda nitpicky. Hard to imagine that they could ever help uncover a bug

Let's keep them turned off then. Have added a section of "intentionally turned off checks"

[MarcoGorelli]

this is unrelated to the pylint changes, but it's really minor so doing it while I'm here

The local hook just above has

    -   id: pip-to-conda
        name: Generate pip dependency from conda
        description: This hook checks if the conda environment.yml and requirements-dev.txt are equal
        language: python
        entry: python scripts/generate_pip_deps_from_conda.py
        files: ^(environment.yml|requirements-dev.txt)$
        pass_filenames: false
        additional_dependencies: [pyyaml, toml]

, so this one might as well take toml as a dep too and they'll share the same env

[MarcoGorelli]

also while I'm touching pylint stuff - adding in a hook only runs the redefined-outer-name hook, which is the pylint check I care the most about. As that's the only check it runs, this is pretty fast (17 seconds in total on my laptop)

the exclude list will be gradually trimmed down as contributors tackle #49656

[jorisvandenbossche]

17 seconds still seems very slow for when committing .. Although I assume that is for the full codebase?

[MarcoGorelli]

17 seconds when running --all-files, per file it should be neglibile running on just pandas/core/generic.py takes .01 seconds

nevermind, that's because it's in the exclude section, so of course it's fast 🤦

If I remove generic.py it from exclude, it takes 2.69s on just that file, which is still kinda slow

[jbrockmendel]

can we add this stuff to the pre-commit hook so i can catch it locally instead of in the CI?

[MarcoGorelli]

could do, but @jorisvandenbossche had brought up that it's a pretty slow hook (esp. compared with the other ones)

I'll see if there's a way to speed it up

[MarcoGorelli]

@jbrockmendel @jorisvandenbossche I don't there's much that can be done to speed up pylint - they say it's expected that it's slow because it's thorough.

And to be fair, it has been helping find actual issues.

To not disrupt local development, but to also help keep catch issues, I'd suggest:

• keep pylint only running in CI with stages: [manual] (though note that you can still trigger it locally if you wish, with pre-commit run pylint --hook-stages manual --all-files)
• turn off the more nitpicky pylint checks such as the no-else-* ones, which probably don't help find issues anyway

[jbrockmendel]

pre-commit run pylint --hook-stages manual --all-files

this will help me out, thanks. The pain point for me is having stuff pass locally and then fail on the CI for some nitpicky reason.

[MarcoGorelli]

cool, so if we remove the nitpicky checks, then local development should still be fast and CI should only fail for reasons which are potential sources of bugs (e.g. redefined-outer-name), but you can still run these checks manually if you wish

and if the are failures because of other reasons deemed to be nitpicky, we can add them back to pyproject.toml in the # intentionally turned off section

any objections to moving along with this?

[jbrockmendel]

any objections to moving along with this?

nope

[jbrockmendel]

pre-commit run pylint --hook-stages manual --all-files

Trying this I'm getting pre-commit: error: unrecognized arguments: --hook-stages manual

[MarcoGorelli]

sorry, typo, I meant --hook-stage

[MarcoGorelli]

any objections to moving along with this?

nope

cool, thanks - if someone approves I'll merge then

[mroeschke]

Just so i understand, the reset of these should be tacked in the future?

[MarcoGorelli]

yup, that's right! It just makes reviews a bit easier - as contributors fix a file, they remove it from this list, and then CI confirms that they, indeed, did fix the issue in that file

[mroeschke]

Looks good. Just a merge conflict and understanding question

[jorisvandenbossche]

What's the idea of adding this one that only runs a specific check if we already have a pylint one for all checks above? (since both are labeled as "manual" stage)

[MarcoGorelli]

this one can't be applied to all files, it has lots of false positives in tests because of fixtures

but, like this, we can turn it on for the non-test files

unfortunately pylint doesn't have a per-file-excludes option like flake8 does

[jorisvandenbossche]

Ah, OK, thanks for the explanation!

[MarcoGorelli]

thanks for reviewing!