Open Stack

On Tue, May 6, 2014 at 6:03 AM, Doug Hellmann
<doug.hellmann at dreamhost.com>wrote:

> On Mon, May 5, 2014 at 9:06 PM, David Stanek <dstanek at dstanek.com> wrote:
> >
> > On Mon, May 5, 2014 at 5:28 PM, Doug Hellmann <
> doug.hellmann at dreamhost.com>
> > wrote:
> >>
> >>
> >> > The assert ones do seem to fit the best practices as I understand
> them,
> >> > but
> >> > I suspect there's going to be quite a bit of work to get projects
> >> > compliant.
> >>
> >> I've seen some work being done on that already, but I don't know how
> >> strongly we care about those specific rules as an overall project.
> >>
> >
> > I created the Keystone blueprint[1] to automate the things we already
> check
> > for in reviews. My motivation was to make it faster for contributors to
> > contribute because they would get feedback before getting a bunch of -1s
> in
> > Gerrit. I also wanted to free up core dev resources so that we can focus
> on
> > more important parts of reviews.
>
> Sure, that makes sense to do in keystone.
>


While local checks are great, when adding them projects should always think
if it makes sense to push them back to hacking itself. Otherwise we end up
with 10 different versions of the same checks.



>
> >
> > I'd be happy to start putting some of these in hacking, but I don't know
> > which rules would be acceptable to all projects. Maybe there is a way to
> > make optional checks that can be enabled in the tox.ini
>

All rules are optional in hacking, but all are enabled by default. If a
project doesn't want to use one they can just disable it.


>
> This is the part I wasn't sure about. Some of the rules look pretty
> useful (e.g., no mutable arguments). Others are more stylistic choices
> (e.g., which type of quotes to use).
>
> Joe Gordon drives the hacking project, and he might be able to give
> more detail about the process for adding new rules there.
>
> Doug
>
> >
> > 1.
> >
> https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation
>


Taking the list from your blueprint, with responses after each.


   -  block comments should have a space after the # (this is already
   enforced for inline comments)
   - pep8 1.5.x has this check already, it will come out in hacking 0.9
      (which is currently blocked on reviews)
   -  mutables should not be used as default args
   - sounds like a good thing to add to hacking. Although a clear why this
      is good is required
   -  % should not be used in log statements
   - Belongs in Hacking, although we have to make sure 'LOG' is a logger
   -  assertNone should be used when using None with assertEqual
   - Why? what is the value here?
   -  spelling check for comments and docstrings
   - I have thought about adding this a few times, but I am -1 on it. The
      only way this can work is as a blacklist of words, since
developers love to
      make up there own words.
      - Having spelling feedback in a non-voting way would be very useful
      though
   -  warn if children that change method signature of their parent
   - Hacking doesn't really do warnings
   -  methods that just pass are useless and should be deleted
   - How do these even get in?
   -  warn on try-except that just passes
   - Hacking doesn't do warnings.
   -  enforce import ordering and spacing
   - So I am really sad to see this here. Hacking 0.9 supports this (once
      again blocked on reviews -- not enough reviewers)
   -  _() should not be used in debug log statements
      - Hacking, once again we need to be rigorous in this check when
      adding to hacking.

I'm sure that as I'm fixing the code to conform to the new checks, I'll
identify additional checks to implement.

(stevemar) if I may add some more:

   - ensure no vim headers
   - I don't think this one belongs in hacking, as once a project removes
      all vim headers I don't think they will be re-added. So I see value of
      having this short term in each project. But not something we
want to check
      for indefinitely
   - assertEqual(len(some_list), 0) should be assertEqual(some_list, [])
   - Why is this better?
   - methods / functions that are unused
   - This isn't really something hacking can do.
   - class properties that are unused
   - Once again this isn't something hacking can really do, or really any
      static analysis in python. pyflakes tries to do some of this.
   - ensure incoming method args are actually used
      - belongs in Pyflakes
   - doc strings - ensure all method args are mentioned (if any are
   mentioned)
   - I have to double check, but I think there is a docstring module for
      flake8 that can do things like this
   - use ' over "
   - Why? And is this a black and white thing that can be enforced.
   - ensure we don't go assigning copyrights to OpenStack Foundation
   - We cannot gate on this, otherwise foundation employees cannot commit
      code.



>
> >
> > --
> > David
> > blog: http://www.traceback.org
> > twitter: http://twitter.com/dstanek
> > www: http://dstanek.com
> >
> > _______________________________________________
> > OpenStack-dev mailing list
> > OpenStack-dev at lists.openstack.org
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140506/d0795738/attachment.html>