*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
<mailto: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 <mailto: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 <mailto: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
<mailto: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 <mailto: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
<mailto: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 <mailto:ja...@potiuk.com>> wrote: >> >> >> >> Why
not +10000 Ash :) ? >> >> >> >> On Wed, Jun 23, 2021 at 9:00 PM
Andrew Godwin >> >> <andrew.god...@astronomer.io.invalid
<mailto: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 <mailto:a...@apache.org>>
wrote: >> >> >> >> >> >> Clearly I'm +7000 on this. >> >> >> >> >>
>> On 23 June 2021 19:38:13 BST, Xinbin Huang
<bin.huan...@gmail.com <mailto: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 <mailto: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
