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

STYLE fix pylint issues #48855

Closed
MarcoGorelli opened this issue Sep 29, 2022 · 105 comments
Closed

STYLE fix pylint issues #48855

MarcoGorelli opened this issue Sep 29, 2022 · 105 comments
Labels
Code Style Code style, linting, code_checks good first issue

Comments

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Sep 29, 2022

In https://github.com/pandas-dev/pandas/pull/48759/files we're introduced pylint, but have turned off its warnings as there's a lot of them:

pandas/pyproject.toml

Lines 36 to 67 in d719840

disable = [
"C",
"R",
"W",
"abstract-class-instantiated",
"access-member-before-definition",
"bad-super-call",
"c-extension-no-member",
"function-redefined",
"import-error",
"inherit-non-class",
"invalid-repr-returned",
"invalid-unary-operand-type",
"misplaced-bare-raise",
"no-member",
"no-method-argument",
"no-name-in-module",
"no-self-argument",
"no-value-for-parameter",
"non-iterator-returned",
"not-an-iterable",
"not-callable",
"redundant-keyword-arg",
"too-many-function-args",
"undefined-variable",
"unexpected-keyword-arg",
"unpacking-non-sequence",
"unsubscriptable-object",
"unsupported-assignment-operation",
"unsupported-membership-test",
"used-before-assignment",
]

Task here is:

  1. pick one of the above warnings / errors, and remove it from pyproject.toml
  2. run pre-commit run pylint --hook-stage manual --all-files. If this already passes, skip to step 4.
  3. fixup any warnings that result
  4. if pre-commit run pylint --hook-stage manual --all-files passes, then stage and commit your changes and open a pull request

If you believe a warning is a false-positive, then it's OK to ignore it in-line, e.g.

df.rename(id, mapper=id)  # pylint: disable=redundant-keyword-arg

Please comment here which pylint warning you'll work on before starting so we don't duplicate work. No need to ask for permission to work on this, and no need to comment "take" as multiple people can work on this concurrently


NOTE: please do not write "closes #48855" in your PR


checks we probably can't turn off:

  • too-many-function-args
  • unsubscriptable-object
  • unsupported-assignment-operation
  • no-name-in-module
  • no-member
  • import-error
  • not-an-iterable
  • invalid-unary-operator-type
  • invalid-name
  • wrong-import-order (and anything else related with imports)
  • unexpected-keyword-argument
  • use-implicit-booleaness-not-len
  • use-implicit-booleaness-not-comparison
  • comparison-with-itself
  • no-else-*
  • line-too-long
  • overridden-final-method
  • singleton-comparison
  • unsupported-membership-test
  • pointless-statement
  • broad-except
  • undefined-variable

If you're not sure whether a check should fit into the above list, please do ask

@MarcoGorelli MarcoGorelli added Code Style Code style, linting, code_checks good first issue labels Sep 29, 2022
@shivamurali
Copy link

shivamurali commented Sep 29, 2022

Hi there! I'll get to work on the "used-before-assignment" warning and remove it from the pyproject.toml file.

EDIT: PR went stale, anyone else is welcome to work on this

@Exoutia

This comment was marked as resolved.

@MarcoGorelli

This comment was marked as resolved.

@Exoutia

This comment was marked as resolved.

@slackline
Copy link

I'll work on removing import-error.

@gulyapulya
Copy link
Contributor

Hi! I am also new to the open source, but I want to contribute, I will work on the "no-method-argument".

@kostyafarber
Copy link
Contributor

Hi! I'll work on inherit-non-class.

@soumilbaldota
Copy link
Contributor

Hi, I will take bad-super-call warning, I have added the pull request above.

@FloHofstetter
Copy link
Contributor

Hi, I will take function-redefined warning.

@rokaicker
Copy link

Hi ! I'm new to open source, but I'd like to tackle the too-many-function-args!

@MarcoGorelli
Copy link
Member Author

thanks @rokaicker - that one's probably too hard to turn off for now, without going through a deprecation cycle, but there's lot's of others to choose

I've added this one to the description as one that's probably not possible to turn off for now

@jetale
Copy link

jetale commented Oct 2, 2022

working on "unsupported-assignment-operation"

@Exoutia
Copy link
Contributor

Exoutia commented Oct 2, 2022

i am working on "no-self-argument"

@shalearkane
Copy link
Contributor

I am working on "unexpected-keyword-arg"

@roadswitcher
Copy link
Contributor

I am working on no-name-in-module

@Leviob
Copy link
Contributor

Leviob commented Oct 2, 2022

Hi, I would like to work on "used-before-assignment," but I'm aware it was already claimed by @shivamurali. Can I submit my own PR for it? Thanks.

@joaomarcoscrs
Copy link

I'll work on the not-callable

@rAwsam
Copy link

rAwsam commented Oct 4, 2022

Hi! I am also new to the open source, but I want to contribute, I will work on the "undefined-variable"

@TejasCreative
Copy link

TejasCreative commented Oct 6, 2022

I'll work on no-member!

@MarcoGorelli
Copy link
Member Author

sorry, --hook-stage

@angularOcean
Copy link
Contributor

angularOcean commented Nov 21, 2022

Thanks --hook-stage fixed the issue. Also, after running a check on unused-argument, there turned up several hundred lines with the issue and from a quick look of a sampling of them, a large proportion of them are false positives i.e. 'col' in:
def time_iteritems_indexing(self): for col in self.df3: self.df3[col]
I can mark this as a false positive but there would be a lot of them. Would it be better to continue keeping this warning turned off?

@MarcoGorelli
Copy link
Member Author

we can probably keep it off

We're probably scraping the barrel here with this issue, I think we've probably enabled most of the most meaningful ones

There's still some files to do in #49656 if you fancy that though

@seanjedi
Copy link
Contributor

Is anyone working on unsupported-membership-test?
Can I take that task?

@angularOcean
Copy link
Contributor

I took a look and I think use-implicit-booleaness-not-comparison is worth working on too

@angularOcean
Copy link
Contributor

I will also work on use-implicit-booleaness-not-len and see if that can enabled as well

@natmokval
Copy link
Contributor

Hi @MarcoGorelli , I checked pylint warning singleton-comparison and suggest keeping it off. Only false-positive warnings are raised on 8 lines (in 4 files); on every line, flake8 warnings, such as noqa: E711 or noqa: E712, are ignored.

For example:
https://github.com/pandas-dev/pandas/blob/main/pandas/tests/series/test_arithmetic.py#L827

https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/indexing/test_where.py#L828

@seanjedi
Copy link
Contributor

seanjedi commented Nov 23, 2022

Hi @MarcoGorelli
I've started to work on unsupported-membership-test if that is alright.
I started working on it here: #49848
I have not opened up the PR for review yet since there are still some issues I am trying to debug, once I have them resolved then I will open up the PR.

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Nov 23, 2022

thanks @seanjedi - let's keep that one off too, I think they're all false positives and we risk annoying contributors

pointless-string-statement might be a good one - strings are sometimes used as comments, and it's probably worth converting them to comments because this check found an issue when I tried it

@CerqueiraMatheus
Copy link
Contributor

I will work on access-member-before-definition

@seanjedi
Copy link
Contributor

seanjedi commented Nov 23, 2022

@MarcoGorelli Alright, thanks for letting me know!
I will work on pointless-string-statement then.
I will create a new branch, and if anyone wants to check what I have done for unsupported-membership-test then they can checkout my branch.
I agree though, that a lot of the changes were not really necessary, and would complicate some of the code.

@seanjedi
Copy link
Contributor

@MarcoGorelli I have worked on implementing the changes for pointless-string-statement here: #49880

@roadswitcher
Copy link
Contributor

roadswitcher commented Nov 25, 2022

Found just over 30 instances of broad-except which appeared to be false positives -- do we want to keep this warning disabled?? (ref PR #49910 for example)

@uzzell
Copy link
Contributor

uzzell commented Nov 25, 2022

I'll take unnecessary-list-index-lookup.

@MarcoGorelli
Copy link
Member Author

Found just over 30 instances of broad-except which appeared to be false positives -- do we want to keep this warning disabled?? (ref PR #49910 for example)

yeah let's do that

@roadswitcher
Copy link
Contributor

Found just over 30 instances of broad-except which appeared to be false positives -- do we want to keep this warning disabled?? (ref PR #49910 for example)

yeah let's do that

PR closed.

@bang128
Copy link
Contributor

bang128 commented Nov 28, 2022

Has anyone taken line-too-long yet? Can I work on it?

@MarcoGorelli
Copy link
Member Author

Thanks everyone for your contributions!

I think we've probably taken the most valuable ones already, so I think we can stop asking for contributions on this one.

If anyone comes across a pylint code which can be removed and which adds then value, then please do submit a PR to fix it! You're still welcome to do that, I just think it's time to close the issue

Thanks all 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks good first issue
Projects
None yet