Skip to content

Modify precommit config yaml and change name of typing step to Typing + pylint. #49527

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

Closed
sudhansubhushanmishra wants to merge 8 commits into from
Closed

Modify precommit config yaml and change name of typing step to Typing + pylint. #49527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7819614
Modify precommit config yaml and change name of typing step to Typing…
sudhansubhushanmishra Nov 4, 2022
cda788d
Fix incorrect syntax.
sudhansubhushanmishra Nov 4, 2022
8256da2
add pylint to environment.yml
Nov 4, 2022
1d1c540
ignore c-extension-no-member
Nov 4, 2022
aa940db
ignore invalid-sequence-index and missing-timeout
Nov 4, 2022
4b05ec3
Merge remote-tracking branch 'upstream/main' into pr/the-lazy-learner…
Nov 4, 2022
c769528
Merge branch 'main' into modify-precommit-config-and-change-name
MarcoGorelli Nov 4, 2022
0974c77
Merge branch 'main' into modify-precommit-config-and-change-name
MarcoGorelli Nov 5, 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
2 changes: 1 addition & 1 deletion .github/workflows/code-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ jobs:
echo "PYTHONPATH=$PYTHONPATH" >> $GITHUB_ENV
if: ${{ steps.build.outcome == 'success' && always() }}

- name: Typing
- name: Typing + pylint
uses: pre-commit/action@v2.0.3
with:
extra_args: --hook-stage manual --all-files
Expand Down
2 changes: 2 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ repos:
rev: v2.15.5
hooks:
- id: pylint
language: system
stages: [manual]
Comment on lines +66 to +67
Copy link
Contributor

@akx akx Nov 4, 2022

Choose a reason for hiding this comment

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

What are the implications of these changes?

Copy link
Member

@MarcoGorelli MarcoGorelli Nov 4, 2022

Choose a reason for hiding this comment

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

it'll run like the mypy one, only in CI

- repo: https://github.com/PyCQA/isort
rev: 5.10.1
hooks:
Expand Down
1 change: 1 addition & 0 deletions environment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ dependencies:
- flake8-bugbear=22.7.1 # used by flake8, find likely bugs
- isort>=5.2.1 # check that imports are in the right order
- mypy=0.981
- pylint=2.15.5
Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 5, 2022

Choose a reason for hiding this comment

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

@MarcoGorelli is there a reason to add pylint to the env file? The check is still run through pre-commit, which will make install it in its own env?

(although that's also the case for black et al, and those are also here, so it's all OK, just wondering)

Copy link
Member

@MarcoGorelli MarcoGorelli Nov 5, 2022

Choose a reason for hiding this comment

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

pylint can do some extra inference if it has everything built (hence a few extra checks that need to be turned off in this PR) - if it's only running in CI, then we might as well use the environment where pandas has been built (like we do for mypy, that one also has language: system)

Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 5, 2022

Choose a reason for hiding this comment

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

Ah, I missed the language: system . Thanks for the explanation!

Copy link
Member

@MarcoGorelli MarcoGorelli Nov 5, 2022

Choose a reason for hiding this comment

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

🤔 though language: system is less reliable, so maybe this should still run in its own little env...yeah let's do that, then the risk of it annoying contributors / maintainers is reduced

- pre-commit>=2.15.0
- pycodestyle # used by flake8
- pyupgrade
Expand Down
3 changes: 3 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,10 @@ exclude = '''
max-line-length = 88
disable = [
"abstract-class-instantiated",
"c-extension-no-member",
"import-error",
"invalid-repr-returned",
"invalid-sequence-index",
"invalid-unary-operand-type",
"no-member",
"no-name-in-module",
Expand Down Expand Up @@ -148,6 +150,7 @@ disable = [
"invalid-overridden-method",
"keyword-arg-before-vararg",
"method-cache-max-size-none",
"missing-timeout",
"non-parent-init-called",
"overridden-final-method",
"pointless-statement",
Expand Down
1 change: 1 addition & 0 deletions requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ flake8==5.0.4
flake8-bugbear==22.7.1
isort>=5.2.1
mypy==0.981
pylint==2.15.5
pre-commit>=2.15.0
pycodestyle
pyupgrade
Expand Down