Enable Cinder hacking checks

Bug #1407162 reported by Ivan Kolodyazhny
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Wishlist
Cindy Pallares

Bug Description

Following PEP8/Flake8 check should be enabled to make core more clean and readable and consistent with OpenStack Hacking rules:
E265: block comment should start with '# ' - makes code more readable
H302: import only modules - OpenStack coding guidelines
H405: multi line docstring summary not separated with an empty line - OpenStack coding guidelines

Ivan Kolodyazhny (e0ne)
Changed in cinder:
assignee: nobody → Ivan Kolodyazhny (e0ne)
Ivan Kolodyazhny (e0ne)
description: updated
Jay Bryant (jsbryant)
Changed in cinder:
importance: Undecided → Medium
status: New → Confirmed
Revision history for this message
Duncan Thomas (duncan-thomas) wrote :

Some of these have been discussed and rejected already (e.g. the git period one) with "Who cares? How does that make anything actually better?". Lots of hacking checks are possible, but unless they do actually make a noticable difference to the code, all they do is annoy developers who have to go fix them up.

Revision history for this message
Ivan Kolodyazhny (e0ne) wrote :

Duncan, I've already removed check for period for commit messages. I'm agree with you about H405, but E265 and H302 are really easy to support during coding and imo it increases code readability

Revision history for this message
Duncan Thomas (duncan-thomas) wrote :

Sound reasonable to me. H302 is something that regularly gets pointed out by manual code reviews, so automating it is definitely a good thing.

Sorry if I came across as overly negative earlier, I was trying to do too many things at once.

Revision history for this message
Ivan Kolodyazhny (e0ne) wrote :

It's that I talk always: if something could be automated it must be automated.

Revision history for this message
Jay Bryant (jsbryant) wrote :

Ivan, I am in support of adding the checks you have listed above. Thanks Ivan.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

Fix proposed to branch: master
Review: https://review.openstack.org/145780

Changed in cinder:
assignee: Ivan Kolodyazhny (e0ne) → Anton Arefiev (aarefiev)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/145821

Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Yuriy Nesenenko (ynesenenko)
Revision history for this message
Huang Zhiteng (zhiteng-huang) wrote :

Duncan, E265 was rejected by you here: https://review.openstack.org/#/c/104493/ with the reason it doesn't actually improve readability. What's changed?

Changed in cinder:
importance: Medium → Low
Changed in cinder:
assignee: Yuriy Nesenenko (ynesenenko) → Anton Arefiev (aarefiev)
Mike Perez (thingee)
Changed in cinder:
milestone: none → kilo-3
Revision history for this message
Ivan Kolodyazhny (e0ne) wrote :

Duncan, could you please confirm that fix E265 is acceptable for you?

Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Yuriy Nesenenko (ynesenenko)
Changed in cinder:
assignee: Yuriy Nesenenko (ynesenenko) → Anton Arefiev (aarefiev)
Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Yuriy Nesenenko (ynesenenko)
Changed in cinder:
assignee: Yuriy Nesenenko (ynesenenko) → Anton Arefiev (aarefiev)
Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Yuriy Nesenenko (ynesenenko)
Changed in cinder:
assignee: Yuriy Nesenenko (ynesenenko) → Anton Arefiev (aarefiev)
Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Yuriy Nesenenko (ynesenenko)
Changed in cinder:
assignee: Yuriy Nesenenko (ynesenenko) → Anton Arefiev (aarefiev)
Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Yuriy Nesenenko (ynesenenko)
Changed in cinder:
assignee: Yuriy Nesenenko (ynesenenko) → Anton Arefiev (aarefiev)
Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Yuriy Nesenenko (ynesenenko)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/145821
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=84afca21fd19d5f78259f0a820258645c1780749
Submitter: Jenkins
Branch: master

commit 84afca21fd19d5f78259f0a820258645c1780749
Author: Yuriy Nesenenko <email address hidden>
Date: Wed Feb 11 13:19:40 2015 +0200

    Fix comments style according to the Hacking Rules

    According to the PEP8(E265) there should be at least
    one space before block comment.

    Change-Id: Ic51f80210becc375b30f0f4e9eeb54995775c817
    Partial-Bug: #1407162

Changed in cinder:
assignee: Yuriy Nesenenko (ynesenenko) → Anton Arefiev (aarefiev)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/145780
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=753f83cd17fe46163be92e40835e21a1960eac06
Submitter: Jenkins
Branch: master

commit 753f83cd17fe46163be92e40835e21a1960eac06
Author: Anton Arefiev <email address hidden>
Date: Thu Jan 8 15:20:13 2015 +0200

    Import only modules: H302

    H302 PEP8 check should be enabled to make core more clean
    and readable and consistent with OpenStack Hacking rules.

    Change-Id: Ie189f2418d12800a46664705eacfc127e7269f45
    Partial-Bug: #1407162

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

Fix proposed to branch: master
Review: https://review.openstack.org/159586

Changed in cinder:
assignee: Anton Arefiev (aarefiev) → Walt Boring (walter-boring)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/159586
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=851557c273b16c26a810bc3add9768f2091f5a76
Submitter: Jenkins
Branch: master

commit 851557c273b16c26a810bc3add9768f2091f5a76
Author: Walter A. Boring IV <email address hidden>
Date: Thu Feb 26 11:43:13 2015 -0800

    Fixes the import for Lefthand driver

    Review 145780 broke the lefthand driver. The author and reviewers
    did not even bother to look at the 3rd party CI results that clearly
    showed that this patch broke the hplefthand driver, due to changing
    the imports. The change removed an import to one of the hplefthandclient
    modules, which was then later accessed.

    Reviewers, please check 3rd party CI when a cinder patch touches a driver.

    Authors of patches, if you touch a driver, you need to check the 3rd party
    CI failures and make sure your patch changes aren't causing the failures.

    Change-Id: Ie38f77fc9498915651c476ea10bc7bd7f49e393d
    Partial-Bug: 1407162

Mike Perez (thingee)
Changed in cinder:
milestone: kilo-3 → liberty-1
Changed in cinder:
assignee: Walt Boring (walter-boring) → nobody
Changed in cinder:
assignee: nobody → Cindy Pallares (cindy-pallaresq)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

Fix proposed to branch: master
Review: https://review.openstack.org/184820

Changed in cinder:
importance: Low → Wishlist
Mike Perez (thingee)
Changed in cinder:
milestone: liberty-1 → liberty-2
Mike Perez (thingee)
Changed in cinder:
milestone: liberty-2 → liberty-3
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/184820
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=cafb5d449f7409a589db53d8d936db8fa422d679
Submitter: Jenkins
Branch: master

commit cafb5d449f7409a589db53d8d936db8fa422d679
Author: Cindy Pallares <email address hidden>
Date: Thu May 21 10:54:34 2015 -0500

    Fix multi-line docstrings to meet hacking rules

    According to the PEP8(H405), multi-line docstring summaries
    should be separated by an empty line.

    Change-Id: I5cd8a9064dcefc504e85946ecdf1f56f10145d35
    Closes-bug: #1407162

Changed in cinder:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in cinder:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in cinder:
milestone: liberty-3 → 7.0.0
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers