airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxime Beauchemin <maximebeauche...@gmail.com>
Subject Re: Airflow Committers: Landscape checks doing more harm than good?
Date Fri, 17 Mar 2017 05:49:51 GMT
Oh btw I think I tried to turn it off before and couldn't. I looked at all
configuration options on both landscape and GH it just kept doing its thing
no matter what. Ended up giving up. Curious to see how it goes this time.

On Mar 16, 2017 8:57 PM, "siddharth anand" <sanand@apache.org> wrote:

> +1 for replacing it with travis linting.
>
> On Thu, Mar 16, 2017 at 7:59 PM, Jeremiah Lowin <jlowin@apache.org> wrote:
>
> > 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