airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Riccomini <criccom...@apache.org>
Subject Re: PEP8, Linting and Code Smells
Date Fri, 03 Jun 2016 01:22:53 GMT
@Maxime +1.. % based really annoys me.

On Thu, Jun 2, 2016 at 6:13 PM, Maxime Beauchemin <
maximebeauchemin@gmail.com> wrote:

> Side note I think I want to remove  "Smell Use % formatting in logging
> functions but pass the % parameters as
> arguments". `%` formatting is outdated and folks are moving away from it in
> favor of "".format() .
>
> On Thu, Jun 2, 2016 at 5:21 PM, Chris Riccomini <criccomini@apache.org>
> wrote:
>
> > The top 6 all seem totally do-able, and shouldn't be disruptive at all,
> > provided small PRs and fast merges.
> >
> > 204 Style Line too long
> > <https://landscape.io/github/apache/incubator-airflow/431/messages/72>
> > 159 Style Import statements should be the first statements in a module
> > <https://landscape.io/github/apache/incubator-airflow/431/messages/373>
> > 91 Smell Unused argument
> > <https://landscape.io/github/apache/incubator-airflow/431/messages/274>
> > 76 Smell Use % formatting in logging functions but pass the % parameters
> as
> > arguments
> > <https://landscape.io/github/apache/incubator-airflow/431/messages/329>
> > 66 Style Variable in function should be lowercase
> > <https://landscape.io/github/apache/incubator-airflow/431/messages/89>
> > 66 Smell Unused variable
> > <https://landscape.io/github/apache/incubator-airflow/431/messages/273>
> >
> >
> > On Thu, Jun 2, 2016 at 5:03 PM, Maxime Beauchemin <
> > maximebeauchemin@gmail.com> wrote:
> >
> > > This is definitely useful! I'd love to get in the 95%+ range. We may
> also
> > > ease some of the rules eventually (it's easy to control in
> > .landscape.yml)
> > > but raw PEP8 is fine by me, though I allowed for up to 90 char per
> line,
> > so
> > > that we can use judgment there.
> > >
> > > Maybe the best approach is to start with "errors" where the solutions
> are
> > > clear, and then proceed by a rule centric approach
> > > <https://landscape.io/github/apache/incubator-airflow/431/messages>,
> it
> > > makes the PRs easier to review. PRs with maybe 50 to 200 line edits
> seem
> > > reasonable.
> > >
> > > All you need is a great soundtrack. I find doing that kind of mindless
> > > repetitive work pretty enjoyable and relaxing.
> > >
> > > Max
> > >
> > > On Thu, Jun 2, 2016 at 4:04 PM, Chris Riccomini <criccomini@apache.org
> >
> > > wrote:
> > >
> > > > @Paul, my opinion is that this is worth while as long as it isn't
> > > > disruptive. I think the way to keep it from being disruptive is:
> > > >
> > > > 1. Small PRs
> > > > 2. Only fix the issues that don't require refactoring (e.g. nit
> picking
> > > on
> > > > line length, spacing, etc)
> > > >
> > > > On Thu, Jun 2, 2016 at 4:01 PM, Paul Rhodes <withnale@outlook.com>
> > > wrote:
> > > >
> > > > > http://landscape.io is already doing this for the project - it's
> > > already
> > > > > integrated with travis and run for every commit/PR
> > > > >
> > > > >
> > > > > I was more concerned about improving the code quality - we already
> > have
> > > > > the metrics.
> > > > >
> > > > >
> > > > > ________________________________
> > > > > From: Bence Nagy <bence@underyx.me>
> > > > > Sent: 02 June 2016 23:52
> > > > > To: Airflow Dev
> > > > > Subject: Re: PEP8, Linting and Code Smells
> > > > >
> > > > > Hey,
> > > > >
> > > > > Please check out http://coala-analyzer.org for static analysis,
> it's
> > > > > awesome! http://gitmate.com is able to run these checks
> > automatically
> > > > for
> > > > > PRs. See https://github.com/coala-analyzer/coala/pull/2220 for a
> > live
> > > > > example (hover over the green checkmark next to the commit hash).
> > > > >
> > > > > I'd love to see as many things copied from coala's CI system as
> > > possible
> > > > -
> > > > > they even have automatic pypi prerelease version deployments for
> each
> > > > > merge!
> > > > >
> > > > > On Thu, Jun 2, 2016 at 2:46 PM Paul Rhodes <withnale@outlook.com>
> > > wrote:
> > > > >
> > > > > > Hi
> > > > > >
> > > > > >
> > > > > > I've asked a few of the committers about this but thought I'd
> ping
> > > this
> > > > > to
> > > > > > the list.
> > > > > >
> > > > > >
> > > > > > We've integrated landscape.io to do automated code checks and
as
> > of
> > > > > right
> > > > > > now it's flagging 8 Errors, 416 Smells and 784 Style alerts.
I've
> > > had a
> > > > > > look at some of these and thought I might pick off some of the
> > lower
> > > > > > hanging fruit in this area, but I'd just like to understand
if
> > > > improving
> > > > > > the static code scores is seen to be of value right now...
> > > > > >
> > > > > >
> > > > > > I'd like to see the scores improve, but like the testing and
the
> > > > changes
> > > > > > to model.py, it's a big job which touches a significant number
of
> > > lines
> > > > > of
> > > > > > code.
> > > > > >
> > > > > >
> > > > > > Of course, it's also fine if we choose to keep the codebase
as-is
> > but
> > > > if
> > > > > > that's the case we should adjust .landscape.yml to a profile
that
> > is
> > > > > > tailored to alert on only the things we care about. (For
> example, I
> > > > would
> > > > > > be in favour of allowing _(\w+) in the pylint dummy variable
> syntax
> > > to
> > > > > > allow context to be maintained for dummy variables))
> > > > > >
> > > > > >
> > > > > > cheers
> > > > > >
> > > > > >
> > > > > > Paul
> > > > > >
> > > > >
> > > >
> > >
> >
>

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