+2 I hate % so much! On Thu, Jun 2, 2016 at 9:23 PM Chris Riccomini <[email protected]> wrote:
> @Maxime +1.. % based really annoys me. > > On Thu, Jun 2, 2016 at 6:13 PM, Maxime Beauchemin < > [email protected]> 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 <[email protected]> > > 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 < > > > [email protected]> 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 < > [email protected] > > > > > > > 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 <[email protected]> > > > > 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 <[email protected]> > > > > > > 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 <[email protected] > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > >
