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 >
