airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremiah Lowin <jlo...@apache.org>
Subject Re: PEP8, Linting and Code Smells
Date Fri, 03 Jun 2016 02:09:26 GMT
+2 I hate % so much!

On Thu, Jun 2, 2016 at 9:23 PM Chris Riccomini <criccomini@apache.org>
wrote:

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