Good point Ash - I think we could indeed (if we decide to disable pylint) do this very thing indeed - identify the "important" aspects and try to find a plugin in flake8 for it. With our codebase, I think it will be rather easy to find if it works well for us (and we could do it as part of the `pylint-disable` PR and iterate until we are happy).
I think it would streamline the development of ours a bit. J. On Thu, Jun 24, 2021 at 11:12 AM Ash Berlin-Taylor <a...@apache.org> wrote: > > Some of the "missing" checks can be added by flake8 plugins -- now obviously > the plugins don't always provide the "no-false-positives" guarantees > > https://github.com/PyCQA/flake8-bugbear is a good one, and is part of the > PyCQA so is "blessed". > > There are many more possible ones we can look at on > https://github.com/DmytroLitvinov/awesome-flake8-extensions, but I think we > should keep the plugins small and only add specific plugins when we identify > a need -- to keep flake8 running fast. > > -ash > > On Thu, Jun 24 2021 at 10:59:58 +0200, Jarek Potiuk <ja...@potiuk.com> wrote: > > Added counts to get better overview: grep -R '# pylint: disable' | sed > 's/.*\(# pylint.*\)/\1/' | sort | uniq -c 1 Binary file > .git/objects/pack/pack-b85cbf9c91a5ba2bc0e0f19ea9bcf9d8193c48d5.pack matches > 3 # pylint: disable=abstract-method 18 # pylint: > disable=assignment-from-no-return 22 # pylint: > disable=attribute-defined-outside-init 195 # pylint: disable=broad-except 2 # > pylint: disable=C0103 2 # pylint: disable=C0103, line-too-long 1 # pylint: > disable=C0111 2 # pylint: disable=C0123 4 # pylint: disable=C0301 3 # pylint: > disable=C0302 18 # pylint: disable=c-extension-no-member 7 # pylint: > disable=c-extension-no-member, no-member 2 # pylint: > disable=c-extension-no-member,no-member 19 # pylint: > disable=comparison-with-callable 1 # pylint: disable=comparison-with-itself 4 > # pylint: disable=consider-using-enumerate 2 # pylint: > disable=consider-using-generator 46 # pylint: disable=consider-using-with 2 # > pylint: disable=consider-using-with,attribute-defined-outside-init 4 # > pylint: disable=cyclic-import 2 # pylint: disable=dangerous-default-value 2 # > pylint: disable=E0012 1 # pylint: disable=E1101 2 # pylint: disable=E1102 3 # > pylint: disable=E1111 8 # pylint: disable=expression-not-assigned 29 # > pylint: disable=global-statement 1 # pylint: > disable=global-statement,global-variable-not-assigned 4 # pylint: > disable=inconsistent-return-statements 1 # pylint: > disable=invalid-bool-returned, bad-option-value 1 # pylint: > disable=invalid-length-returned 123 # pylint: disable=invalid-name 2 # > pylint: disable=invalid-name,missing-docstring 2 # pylint: > disable=invalid-name, unused-argument 1 # pylint: > disable=invalid-sequence-index 2 # pylint: disable=invalid-str-returned 2 # > pylint: disable=len-as-condition 20 # pylint: disable=line-too-long 3 # > pylint: disable=line-too-long # noqa 4 # pylint: disable=logging-not-lazy 4 # > pylint: disable=lost-exception 26 # pylint: disable=maybe-no-member 21 # > pylint: disable=missing-docstring 1 # pylint: disable=missing-docstring, > redefined-outer-name 12 # pylint: disable=missing-function-docstring 6 # > pylint: disable=missing-kwoa 4 # pylint: disable-msg=too-many-arguments 2 # > pylint: disable=no-else-break 2 # pylint: disable=no-else-continue 741 # > pylint: disable=no-member 1 # pylint: disable=no-member,invalid-name 3 # > pylint: disable=no-member,line-too-long 1 # pylint: disable=no-member, > maybe-no-member 3 # pylint: disable=no-member # noqa 2 # pylint: > disable=no-method-argument 58 # pylint: disable=no-name-in-module 1 # pylint: > disable=no-name-in-module,wrong-import-order 2 # pylint: > disable=non-parent-init-called 6 # pylint: disable=no-self-argument 1 # > pylint: disable=not-a-mapping 6 # pylint: disable=not-callable 222 # pylint: > disable=no-value-for-parameter 6 # pylint: disable=pointless-statement 123 # > pylint: disable=protected-access 2 # pylint: > disable=protected-access,c-extension-no-member,no-member 2 # pylint: > disable=protected-access,no-member 2 # pylint: disable=R0401 2 # pylint: > disable=R0902,R0904 1 # pylint: disable=R0904, C0111 1 # pylint: > disable=R0904, C0111, C0302 3 # pylint: disable=R0904, C0302 3 # pylint: > disable=R0913, C0302 2 # pylint: disable=raising-format-tuple 14 # pylint: > disable=redefined-builtin 1 # pylint: > disable=redefined-builtin,unused-argument 2 # pylint: > disable=redefined-builtin,unused-argument,too-many-arguments 14 # pylint: > disable=redefined-outer-name 6 # pylint: > disable=redefined-outer-name,reimported 7 # pylint: > disable=redefined-outer-name,reimported,unused-import 1 # pylint: > disable=redefined-outer-name,unused-argument 2 # pylint: > disable=reimported,unused-import,redefined-outer-name 2 # pylint: > disable=self-assigning-variable 13 # pylint: disable=signature-differs 2 # > pylint: disable=signature-differs # noqa: D403 2 # pylint: > disable=singleton-comparison 4 # pylint: disable=subprocess-popen-preexec-fn > 2 # pylint: disable=subprocess-popen-preexec-fn,consider-using-with 5 # > pylint: disable=subprocess-run-check 7 # pylint: > disable=super-init-not-called 2 # pylint: disable=syntax-error 3 # pylint: > disable=too-few-public-methods 17 # pylint: disable=too-many-ancestors 270 # > pylint: disable=too-many-arguments 2 # pylint: > disable=too-many-arguments,inconsistent-return-statements 6 # pylint: > disable=too-many-arguments, too-many-locals 26 # pylint: > disable=too-many-arguments,too-many-locals 2 # pylint: > disable=too-many-arguments,too-many-locals,too-many-branches 2 # pylint: > disable=too-many-arguments,too-many-locals, too-many-statements 2 # pylint: > disable=too-many-boolean-expressions 4 # pylint: disable=too-many-branches 1 > # pylint: disable=too-many-function-args 78 # pylint: > disable=too-many-instance-attributes 2 # pylint: > disable=too-many-instance-attributes, too-many-arguments 2 # pylint: > disable=too-many-instance-attributes,too-many-arguments,too-many-branches 2 # > pylint: disable=too-many-instance-attributes,too-many-locals 2 # pylint: > disable=too-many-instance-attributes,too-many-public-methods 14 # pylint: > disable=too-many-lines 21 # pylint: disable=too-many-locals 6 # pylint: > disable=too-many-locals,too-many-arguments 3 # pylint: > disable=too-many-locals,too-many-arguments,invalid-name 3 # pylint: > disable=too-many-locals,too-many-arguments, too-many-branches 4 # pylint: > disable=too-many-locals,too-many-statements 71 # pylint: > disable=too-many-nested-blocks 9 # pylint: disable=too-many-public-methods 18 > # pylint: disable=too-many-return-statements 7 # pylint: > disable=too-many-statements 5 # pylint: disable=undefined-variable 11 # > pylint: disable=unexpected-keyword-arg 4 # pylint: disable=ungrouped-imports > 4 # pylint: disable=unidiomatic-typecheck 1 # pylint: > disable=unnecessary-lambda 18 # pylint: disable=unsubscriptable-object 4 # > pylint: disable=unsupported-membership-test 1 # pylint: disable = > unused-argument 188 # pylint: disable=unused-argument 1 # pylint: > disable=unused-argument,invalid-name 1 # pylint: > disable=unused-argument,line-too-long 2 # pylint: disable=unused-argument # > pragma: no cover 3 # pylint: > disable=unused-argument,too-many-arguments,too-many-locals 2 # pylint: > disable=unused-argument, unused-variable 549 # pylint: disable=unused-import > 2 # pylint: disable=unused-import,line-too-long 2 # pylint: > disable=unused-import # noqa 2 # pylint: disable=unused-import # noqa: F401 4 > # pylint: disable=unused-import # noqa: F401 8 # pylint: > disable=unused-variable 1 # pylint: disable=unused-variable # noqa 4 # > pylint: disable=using-constant-test 1 # pylint: disable=W0104 6 # pylint: > disable=W0106 1 # pylint: disable=W0108 1 # pylint: disable=W0143 2 # pylint: > disable=W0201 20 # pylint: disable=W0212 1 # pylint: disable=W0612 4 # > pylint: disable=W0703 1 # pylint: disable=wildcard-import 5 # pylint: > disable=wrong-import-order 5 # pylint: disable=wrong-import-position On Thu, > Jun 24, 2021 at 10:53 AM Jarek Potiuk <ja...@potiuk.com> wrote: > > It could be survivor bias, but at least in my memory pretty much every time > I've seen something really serious, it's been also reported by flake8 (which > on its own is a combination of pep8, pyflakes + circular complexity). Let me > explain a bit more context that built up in my mind over the last few months > which made me change my mind. I think for such a tool, it is important to > have as few false-positives as possible. If you have a lot of > false-positives, we are basically trading both - contributor's and reviewers > time on analysing and explaining that "this is false positive" with the > potential of introducing a mistake like a typo. As a reviewer, this is a > recurring pattern recently that the contributor complains about a "new" > problem reported in the code they did not touch and then the reviewer has to > get engaged and explain, and iterate often resulting in having to rebase the > PR (sometimes again and again). When I look at our code: grep -R '# pylint: > disable' | wc -l 3370 find . -name '*.py' | wc -l 4924 We have > 3000 false > positives in our code and almost 5000 python files. Those false positives are > time lost by both contributors and reviewers. Those are the places where > contributor and reviewer jointly decided that it's not worth to fix the > problem reported by pylunt. Is 3370/4920 a lot ? I think so. Yes it might > mean we should disable some rules possibly. But when I look at those rules to > disable, those are the very ones that we would like to be able to discover. > The list of those unique disables at the end of the mail. If you look at the > pyflakes (which is one of the three tools in flake8) -> "Pyflakes makes a > simple promise: it will never complain about style, and it will try very, > very hard to never emit false positives." . I think it delivers. Pylint makes > no such promise (and delivers as well). J. #### Currently disabled pylint > rules: grep -R '# pylint: disable' | sed 's/.*\(# pylint.*\)/\1/' | sort | > uniq # pylint: disable=abstract-method # pylint: > disable=assignment-from-no-return # pylint: > disable=attribute-defined-outside-init # pylint: disable=broad-except # > pylint: disable=C0103 # pylint: disable=C0103, line-too-long # pylint: > disable=C0111 # pylint: disable=C0123 # pylint: disable=C0301 # pylint: > disable=C0302 # pylint: disable=c-extension-no-member # pylint: > disable=c-extension-no-member, no-member # pylint: > disable=c-extension-no-member,no-member # pylint: > disable=comparison-with-callable # pylint: disable=comparison-with-itself # > pylint: disable=consider-using-enumerate # pylint: > disable=consider-using-generator # pylint: disable=consider-using-with # > pylint: disable=consider-using-with,attribute-defined-outside-init # pylint: > disable=cyclic-import # pylint: disable=dangerous-default-value # pylint: > disable=E0012 # pylint: disable=E1101 # pylint: disable=E1102 # pylint: > disable=E1111 # pylint: disable=expression-not-assigned # pylint: > disable=global-statement # pylint: > disable=global-statement,global-variable-not-assigned # pylint: > disable=inconsistent-return-statements # pylint: > disable=invalid-bool-returned, bad-option-value # pylint: > disable=invalid-length-returned # pylint: disable=invalid-name # pylint: > disable=invalid-name,missing-docstring # pylint: disable=invalid-name, > unused-argument # pylint: disable=invalid-sequence-index # pylint: > disable=invalid-str-returned # pylint: disable=len-as-condition # pylint: > disable=line-too-long # pylint: disable=line-too-long # noqa # pylint: > disable=logging-not-lazy # pylint: disable=lost-exception # pylint: > disable=maybe-no-member # pylint: disable=missing-docstring # pylint: > disable=missing-docstring, redefined-outer-name # pylint: > disable=missing-function-docstring # pylint: disable=missing-kwoa # pylint: > disable-msg=too-many-arguments # pylint: disable=no-else-break # pylint: > disable=no-else-continue # pylint: disable=no-member # pylint: > disable=no-member,invalid-name # pylint: disable=no-member,line-too-long # > pylint: disable=no-member, maybe-no-member # pylint: disable=no-member # noqa > # pylint: disable=no-method-argument # pylint: disable=no-name-in-module # > pylint: disable=no-name-in-module,wrong-import-order # pylint: > disable=non-parent-init-called # pylint: disable=no-self-argument # pylint: > disable=not-a-mapping # pylint: disable=not-callable # pylint: > disable=no-value-for-parameter # pylint: disable=pointless-statement # > pylint: disable=protected-access # pylint: > disable=protected-access,c-extension-no-member,no-member # pylint: > disable=protected-access,no-member # pylint: disable=R0401 # pylint: > disable=R0902,R0904 # pylint: disable=R0904, C0111 # pylint: disable=R0904, > C0111, C0302 # pylint: disable=R0904, C0302 # pylint: disable=R0913, C0302 # > pylint: disable=raising-format-tuple # pylint: disable=redefined-builtin # > pylint: disable=redefined-builtin,unused-argument # pylint: > disable=redefined-builtin,unused-argument,too-many-arguments # pylint: > disable=redefined-outer-name # pylint: > disable=redefined-outer-name,reimported # pylint: > disable=redefined-outer-name,reimported,unused-import # pylint: > disable=redefined-outer-name,unused-argument # pylint: > disable=reimported,unused-import,redefined-outer-name # pylint: > disable=self-assigning-variable # pylint: disable=signature-differs # pylint: > disable=signature-differs # noqa: D403 # pylint: disable=singleton-comparison > # pylint: disable=subprocess-popen-preexec-fn # pylint: > disable=subprocess-popen-preexec-fn,consider-using-with # pylint: > disable=subprocess-run-check # pylint: disable=super-init-not-called # > pylint: disable=syntax-error # pylint: disable=too-few-public-methods # > pylint: disable=too-many-ancestors # pylint: disable=too-many-arguments # > pylint: disable=too-many-arguments,inconsistent-return-statements # pylint: > disable=too-many-arguments, too-many-locals # pylint: > disable=too-many-arguments,too-many-locals # pylint: > disable=too-many-arguments,too-many-locals,too-many-branches # pylint: > disable=too-many-arguments,too-many-locals, too-many-statements # pylint: > disable=too-many-boolean-expressions # pylint: disable=too-many-branches # > pylint: disable=too-many-function-args # pylint: > disable=too-many-instance-attributes # pylint: > disable=too-many-instance-attributes, too-many-arguments # pylint: > disable=too-many-instance-attributes,too-many-arguments,too-many-branches # > pylint: disable=too-many-instance-attributes,too-many-locals # pylint: > disable=too-many-instance-attributes,too-many-public-methods # pylint: > disable=too-many-lines # pylint: disable=too-many-locals # pylint: > disable=too-many-locals,too-many-arguments # pylint: > disable=too-many-locals,too-many-arguments,invalid-name # pylint: > disable=too-many-locals,too-many-arguments, too-many-branches # pylint: > disable=too-many-locals,too-many-statements # pylint: > disable=too-many-nested-blocks # pylint: disable=too-many-public-methods # > pylint: disable=too-many-return-statements # pylint: > disable=too-many-statements # pylint: disable=undefined-variable # pylint: > disable=unexpected-keyword-arg # pylint: disable=ungrouped-imports # pylint: > disable=unidiomatic-typecheck # pylint: disable=unnecessary-lambda # pylint: > disable=unsubscriptable-object # pylint: disable=unsupported-membership-test > # pylint: disable = unused-argument # pylint: disable=unused-argument # > pylint: disable=unused-argument,invalid-name # pylint: > disable=unused-argument,line-too-long # pylint: disable=unused-argument # > pragma: no cover # pylint: > disable=unused-argument,too-many-arguments,too-many-locals # pylint: > disable=unused-argument, unused-variable # pylint: disable=unused-import # > pylint: disable=unused-import,line-too-long # pylint: disable=unused-import # > noqa # pylint: disable=unused-import # noqa: F401 # pylint: > disable=unused-import # noqa: F401 # pylint: disable=unused-variable # > pylint: disable=unused-variable # noqa # pylint: disable=using-constant-test > # pylint: disable=W0104 # pylint: disable=W0106 # pylint: disable=W0108 # > pylint: disable=W0143 # pylint: disable=W0201 # pylint: disable=W0212 # > pylint: disable=W0612 # pylint: disable=W0703 # pylint: > disable=wildcard-import # pylint: disable=wrong-import-order # pylint: > disable=wrong-import-position On Thu, Jun 24, 2021 at 1:57 AM Tomasz Urbaszek > <turbas...@apache.org> wrote: > > -0.5 from me. Black and flake8 does not > guarantee the same quality and consistency of code as pylint (for example > using if list instead of if len(list) > 0 ). > > > I can't remember the last > time where pylint prevented any real error > > Well, isn't it a survivor bias > (the one usually represented by this picture of airplane)? > > Pylint is slow > and sometimes brittle but I think it may help. What I would consider is to > keep pylint for providers - this code in particular is created and changed by > plenty of people with different backgrounds and standards. > > Tomek > > On > Wed, 23 Jun 2021 at 21:45, Jarek Potiuk <ja...@potiuk.com> wrote: >> >> Just > to be precise - the vote will end on Monday 28th at 8.30 PM CEST >> (to > account for weekend) ... >> >> J. >> >> On Wed, Jun 23, 2021 at 9:29 PM Kaxil > Naik <kaxiln...@gmail.com> wrote: >> > >> > +1 -- Pylint is proving to be a > headache and is very very slow locally this days >> > >> > On Wed, Jun 23, > 2021 at 8:02 PM Jarek Potiuk <ja...@potiuk.com> wrote: >> >> >> >> Why not > +10000 Ash :) ? >> >> >> >> On Wed, Jun 23, 2021 at 9:00 PM Andrew Godwin >> > >> <andrew.god...@astronomer.io.invalid> wrote: >> >> > >> >> > I can't cast > a binding vote on this, but I would also like to chime in with support for > this move. We have plenty of protection from everything else, and while we > could try to go through pylint and disable every superfluous check, I'm not > sure it's worth it. >> >> > >> >> > Andrew >> >> > >> >> > On Wed, Jun 23, > 2021 at 12:49 PM Ash Berlin-Taylor <a...@apache.org> wrote: >> >> >> >> >> >> > Clearly I'm +7000 on this. >> >> >> >> >> >> On 23 June 2021 19:38:13 BST, > Xinbin Huang <bin.huan...@gmail.com> wrote: >> >> >>> >> >> >>> I would like > to deprecate it too, so count +1 from me. One question I have is: do we have > any ways to detect and prevent cyclic imports? >> >> >>> >> >> >>> On Wed, > Jun 23, 2021 at 11:30 AM Jarek Potiuk <ja...@potiuk.com> wrote: >> >> >>>> >> > >> >>>> I think this subject has been raised a few times (last time by Ash). > >> >> >>>> Finally I grew up to embrace it as well. >> >> >>>> >> >> >>>> I > think I am also fed-up by random pylint errors. Last time >> >> >>>> > https://github.com/apache/airflow/pull/15634/checks?check_run_id=2896761068 > >> >> >>>> >> >> >>>> We have many, many pylint exceptions all over our code. > I can't >> >> >>>> remember the last time where pylint prevented any real > error. As Ash >> >> >>>> (rightfully) mentioned in latest discussion on slack > - we have >> >> >>>> mypy/flake/isort/black which report (and fix) vast > majority of things >> >> >>>> pylint reports. >> >> >>>> >> >> >>>> I think > this last error was the final drop for me. >> >> >>>> >> >> >>>> Should we > remove pylint ? >> >> >>>> >> >> >>>> Consider it +1 from my side. >> >> >>>> > >> >> >>>> J . >> >> >>>> >> >> >>>> >> >> >>>> -- >> >> >>>> +48 660 796 129 > >> >> >> >> >> >> >> >> -- >> >> +48 660 796 129 >> >> >> >> -- >> +48 660 > 796 129 -- +48 660 796 129 > > -- > +48 660 796 129 -- +48 660 796 129
