+1 Most of the time I disabled it locally and only ran it on CI.
czw., 24 cze 2021 o 20:20 Tomasz Urbaszek <turbas...@apache.org> napisał(a): > > While I fully agree with all the points about pylint being slow and > cumbersome I still keep my -0.5, which if I correctly understand is not > blocking but is just expression of my opinion :D > > Thanks, > Tomek > > On Thu, 24 Jun 2021 at 13:07, Jarek Potiuk <ja...@potiuk.com> wrote: >> >> Just to give an example of the problems we have to deal with re: false >> positives. >> >> I ALMOST had a working Python 3.9 change this morning. ALMOST because >> when I run it everything worked except about hundred new 'no-member' >> errors in a file that I have >> not touched (and it has no relation whatsoever with the changes I made). >> >> No new pylint was released. Just other, unrelated code was changed: >> >> https://github.com/potiuk/airflow/runs/2903575190?check_suite_focus=true#step:9:48 >> >> This is something that recently started to occur almost daily - for >> various people. This is mightily annoying and causes even more delays >> and reruns of the builds (thus money) for really bogus reasons. >> >> J. >> >> On Thu, Jun 24, 2021 at 11:33 AM Ash Berlin-Taylor <a...@apache.org> wrote: >> > >> > *23m23s on our more-powerfull self-hosted runners. >> > >> > On Thu, Jun 24 2021 at 10:32:43 +0100, Ash Berlin-Taylor <a...@apache.org> >> > wrote: >> > >> > The biggest advantage to me: reducing test time on CI and pre-commit. >> > >> > Pylint is slow. >> > >> > Oh my god, I just checked and it's even slower than I realised >> > >> > 46m25s on Github Runners, 23m23s for the "pylint" job. >> > >> > https://github.com/apache/airflow/runs/2902755059?check_suite_focus=true >> > https://github.com/apache/airflow/runs/2900159473?check_suite_focus=true >> > >> > There is no way pylint is worth that much test time! >> > >> > -ash >> > >> > On Thu, Jun 24 2021 at 11:22:53 +0200, Jarek Potiuk <ja...@potiuk.com> >> > wrote: >> > >> > 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 >> >> >> >> -- >> +48 660 796 129
