Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Nov 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
04a1eeb
ignore no-else-*, comment some checks as intentionally turned off, sh…
Nov 17, 2022
93b5793
add redefined-outer-name check, but turn off in remaining files
Nov 17, 2022
6ec1012
keep redefined-outer-name to manual stage
Nov 19, 2022
e238fbb
Merge remote-tracking branch 'upstream/main' into no-else
Nov 19, 2022
312d64f
update exclude list
Nov 19, 2022
156ef11
reword
Nov 19, 2022
038a7e8
Merge remote-tracking branch 'upstream/main' into no-else
Nov 23, 2022
18f935e
Merge remote-tracking branch 'upstream/main' into no-else
Nov 23, 2022
255317a
fixup after merge
Nov 23, 2022
157ac22
fixup again
Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/code-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ jobs:
- name: Run pre-commit
uses: pre-commit/[email protected]

typing_and_docstring_validation:
name: Docstring and typing validation
docstring_typing_pylint:
name: Docstring validation, typing, and pylint
runs-on: ubuntu-latest
defaults:
run:
Expand Down
26 changes: 25 additions & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,30 @@ repos:
hooks:
- id: pylint
stages: [manual]
- repo: https://github.com/pycqa/pylint
rev: v2.15.5
hooks:
- id: pylint
files: ^pandas/
exclude: |
(?x)
^pandas/tests # leave turned off
|/_testing/ # leave turned off
|^pandas/util/_test_decorators\.py # leave turned off
|^pandas/_version\.py # leave turned off
|^pandas/conftest\.py
|^pandas/core/generic\.py
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

|^pandas/core/internals/concat\.py
|^pandas/core/reshape/merge\.py
|^pandas/core/tools/datetimes\.py
|^pandas/formats/format\.py
|^pandas/io/formats/format\.py
|^pandas/io/formats/style\.py
|^pandas/io/json/_json\.py
|^pandas/io/xml\.py
|^pandas/util/_decorators\.py
|^pandas/util/_doctools\.py
args: [--disable=all, --enable=redefined-outer-name]
Copy link
Member Author

@MarcoGorelli MarcoGorelli Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@MarcoGorelli MarcoGorelli Nov 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

- repo: https://github.com/PyCQA/isort
rev: 5.10.1
hooks:
Expand Down Expand Up @@ -201,7 +225,7 @@ repos:
entry: python scripts/sync_flake8_versions.py
files: ^(\.pre-commit-config\.yaml|environment\.yml)$
pass_filenames: false
additional_dependencies: [pyyaml]
additional_dependencies: [pyyaml, toml]
Comment on lines -204 to +223
Copy link
Member Author

Choose a reason for hiding this comment

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

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

- id: title-capitalization
name: Validate correct capitalization among titles in documentation
entry: python scripts/validate_rst_title_capitalization.py
Expand Down
42 changes: 25 additions & 17 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,51 +58,60 @@ exclude = '''
[tool.pylint.messages_control]
max-line-length = 88
disable = [
"abstract-class-instantiated",
"c-extension-no-member",
# intentionally turned off
"comparison-with-itself",
"import-error",
"import-outside-toplevel",
"invalid-name",
"invalid-unary-operand-type",
"invalid-unary-operator-type",
"no-else-continue",
"no-else-raise",
"no-else-return",
"no-member",
"no-name-in-module",
"no-value-for-parameter",
"not-an-iterable",
"not-callable",
"redundant-keyword-arg",
"too-many-function-args",
"undefined-variable",
"unexpected-keyword-argument",
"unexpected-keyword-arg",
"unpacking-non-sequence",
"ungrouped-imports",
"unsubscriptable-object",
"unsupported-assignment-operation",
"unused-import",
"use-implicit-booleaness-not-comparison",
"use-implicit-booleaness-not-len",
"wrong-import-order",
"wrong-import-order",
"wrong-import-position",

# misc
"abstract-class-instantiated",
"c-extension-no-member",
"no-value-for-parameter",
"not-callable",
"redundant-keyword-arg",
"undefined-variable",
"unpacking-non-sequence",
"unsupported-membership-test",

# pylint type "C": convention, for programming standard violation
"import-outside-toplevel",
"invalid-name",
"line-too-long",
"missing-class-docstring",
"missing-function-docstring",
"missing-module-docstring",
"singleton-comparison",
"too-many-lines",
"typevar-name-incorrect-variance",
"ungrouped-imports",
"unidiomatic-typecheck",
"unnecessary-dunder-call",
"unnecessary-lambda-assignment",
"use-implicit-booleaness-not-comparison",
"use-implicit-booleaness-not-len",
"wrong-import-order",
"wrong-import-position",

# pylint type "R": refactor, for bad code smell
"comparison-with-itself",
"consider-using-ternary",
"consider-using-with",
"cyclic-import",
"duplicate-code",
"inconsistent-return-statements",
"no-else-return",
"redefined-argument-from-local",
"too-few-public-methods",
"too-many-ancestors",
Expand Down Expand Up @@ -155,7 +164,6 @@ disable = [
"unnecessary-lambda",
"unspecified-encoding",
"unused-argument",
"unused-import",
"unused-variable",
"using-constant-test",
"useless-parent-delegation"
Expand Down