airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremiah Lowin <jlo...@apache.org>
Subject Re: Airflow Committers: Landscape checks doing more harm than good?
Date Fri, 17 Mar 2017 02:59:17 GMT
FWIW I recently started using yapf (https://github.com/google/yapf) with a
slightly custom config to format all of my projects. Rather than alert to
discrete linting errors and concrete style rules (like PEP8) -- things I'm
sure we all do anyway -- it reformats all code in compliance with your
chosen style rules. It even reformats code that is already PEP8 compliant
to make it more "pythonic" (and still PEP8 compliant). Basically: if you
like (or create) a yapf style, it takes care of all the hard reformatting
work and produces pleasing, consistent results. /plug

On Thu, Mar 16, 2017 at 8:42 PM Maxime Beauchemin <
maximebeauchemin@gmail.com> wrote:

> Let's wire a custom a linter command that can be called locally and respect
> an agreed upon set of parameters (pylint + config file, based off of our
> current .landscape.yml ).
>
> flake8 is far from being as good as pylint and can't be customized much
> AFAICT, but variations on the command bellow can help you lint [only] your
> PR:
>
> `git diff HEAD^ | flake8 --diff`
>
> It's a good thing to integrate in your workflow until we get an equivalent
> pylint command/config
>
> On Thu, Mar 16, 2017 at 5:03 PM, Alex Guziel <alex.guziel@airbnb.com
> .invalid
> > wrote:
>
> > +1 also
> >
> > We have code review already and the amount of false positives makes this
> > useless.
> >
> > On Thu, Mar 16, 2017 at 5:02 PM, Maxime Beauchemin <
> > maximebeauchemin@gmail.com> wrote:
> >
> > > +1 as well
> > >
> > > I'm disappointed because the service is inches away from getting
> > everything
> > > right. As Bolke said, behind the cover it's little more than pylint,
> git
> > > hooks, and a somewhat-fancy ui.
> > >
> > > Operationally it's been getting in the way.
> > >
> > > There's a way to pipe the output of git diff into pylint and check
> > whether
> > > the touched lines need linting, in which case we should break the
> build.
> > > This could run in it's own slot in the Travis build matrix.
> > >
> > > Max
> > >
> > > On Thu, Mar 16, 2017 at 4:51 PM, Bolke de Bruin <bdbruin@gmail.com>
> > wrote:
> > >
> > > > We can do it in Travis’ afaik. We should replace it.
> > > >
> > > > So +1.
> > > >
> > > > B.
> > > >
> > > > > On 16 Mar 2017, at 16:48, Jeremiah Lowin <jlowin@apache.org>
> wrote:
> > > > >
> > > > > This may be an unpopular opinion, but most Airflow PRs have a
> little
> > > red
> > > > > "x" next to them not because they have failing unit tests, but
> > because
> > > > the
> > > > > Landscape check has decided they introduce bad code.
> > > > >
> > > > > Unfortunately Landscape is often wrong -- here it is telling me my
> > > latest
> > > > > PR introduced no less than 30 errors... in files I didn't touch!
> > > > > https://github.com/apache/incubator-airflow/pull/2157 (however, it
> > > > gives me
> > > > > credit for fixing 23 errors in those same files, so I've got that
> > going
> > > > for
> > > > > me... which is nice.)
> > > > >
> > > > > The upshot is that Github's "health" indicator can be swayed by
> minor
> > > or
> > > > > erroneous issues, and therefore it serves little purpose other than
> > > > making
> > > > > it look like every PR is bad. This creates committer fatigue, since
> > > every
> > > > > PR needs to be parsed to see if it actually is OK or not.
> > > > >
> > > > > Don't get me wrong, I'm all for proper style and on occasion
> > Landscape
> > > > has
> > > > > pointed out problems that I've gone and fixed. But on the whole,
I
> > > > believe
> > > > > that having it as part of our red / green PR evaluation -- equal
to
> > and
> > > > > often superseding unit tests -- is harmful. I'd much rather be able
> > to
> > > > scan
> > > > > the PR list and know unequivocally that "green" indicates ready to
> > > merge.
> > > > >
> > > > > J
> > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message