+1 for replacing it with travis linting. On Thu, Mar 16, 2017 at 7:59 PM, Jeremiah Lowin <[email protected]> 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 < > [email protected]> 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 <[email protected] > > .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 < > > > [email protected]> 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 <[email protected]> > > > wrote: > > > > > > > > > We can do it in Travis’ afaik. We should replace it. > > > > > > > > > > So +1. > > > > > > > > > > B. > > > > > > > > > > > On 16 Mar 2017, at 16:48, Jeremiah Lowin <[email protected]> > > 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 > > > > > > > > > > > > > > > > > > > >
