You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@airflow.apache.org by Jeremiah Lowin <jl...@apache.org> on 2017/03/16 23:48:20 UTC
Airflow Committers: Landscape checks doing more harm than good?
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
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Arthur Wiedmer <ar...@gmail.com>.
I just turned it off.
I will look into enabling the right options for our Travis build.
Best,
Arthur
On Thu, Mar 16, 2017 at 5:31 PM, Jeremiah Lowin <jl...@apache.org> wrote:
> Sounds like a probable consensus. I think this is a good time for me to
> admit I have no idea how to turn it off or move it to Travis.
>
> Could anyone please volunteer to take that on?
>
> On Thu, Mar 16, 2017 at 8:11 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 <bd...@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 <jl...@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
> > > >
> > > >
> > >
> >
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Jeremiah Lowin <jl...@apache.org>.
Sounds like a probable consensus. I think this is a good time for me to
admit I have no idea how to turn it off or move it to Travis.
Could anyone please volunteer to take that on?
On Thu, Mar 16, 2017 at 8:11 PM Alex Guziel <al...@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 <bd...@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 <jl...@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
> > >
> > >
> >
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Maxime Beauchemin <ma...@gmail.com>.
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" <sa...@apache.org> wrote:
> +1 for replacing it with travis linting.
>
> On Thu, Mar 16, 2017 at 7:59 PM, Jeremiah Lowin <jl...@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 <jl...@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
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by siddharth anand <sa...@apache.org>.
+1 for replacing it with travis linting.
On Thu, Mar 16, 2017 at 7:59 PM, Jeremiah Lowin <jl...@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 <bd...@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 <jl...@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
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Jeremiah Lowin <jl...@apache.org>.
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 <bd...@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 <jl...@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
> > > >
> > > >
> > >
> >
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Maxime Beauchemin <ma...@gmail.com>.
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 <bd...@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 <jl...@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
> > >
> > >
> >
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Alex Guziel <al...@airbnb.com.INVALID>.
+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 <bd...@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 <jl...@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
> >
> >
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Maxime Beauchemin <ma...@gmail.com>.
+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 <bd...@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 <jl...@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
>
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Dan Davydov <da...@airbnb.com.INVALID>.
+1 as well though I have found it useful on larger PRs to help me catch
some issues so it probably makes sense to make to add the travis linting at
the same time we remove it in landscape. Not sure how much usability we
lose by losing the landscape UI but I like that all of the errors would be
in one place.
On Thu, Mar 16, 2017 at 4:51 PM, Bolke de Bruin <bd...@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 <jl...@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
>
>
Re: Airflow Committers: Landscape checks doing more harm than good?
Posted by Bolke de Bruin <bd...@gmail.com>.
We can do it in Travis’ afaik. We should replace it.
So +1.
B.
> On 16 Mar 2017, at 16:48, Jeremiah Lowin <jl...@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