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 > > > > > > > > > > > > > >
