Begin pep8 fixups #103

edunham · 2017-03-14T23:46:00Z

Partial pep8ening to get some feedback on the various styles of breaking up long lines that I've been using.

@larsbergstrom, @Manishearth, could I get you to skim the diff and comment on any changes that you feel are a step in the wrong direction?

This change is

larsbergstrom · 2017-03-18T14:52:32Z

@aneeshusa Could you please take a look and briefly comment on the changes? You seem to have a stronger sense of "good Python style" than I do :-) Thanks!

aneeshusa

👍 for making this more pep8 friendly! This looks OK at a glance but I personally much prefer block style indent.

aneeshusa · 2017-03-20T04:55:25Z

homu/server.py

@@ -1,7 +1,8 @@
 import hmac
 import json
 import urllib.parse
-from .main import PullReqState, parse_commands, db_query, INTERRUPTED_BY_HOMU_RE, synchronize
+from .main import (PullReqState, parse_commands, db_query,
+                   INTERRUPTED_BY_HOMU_RE, synchronize)


I much prefer block style indents (with trailing commas) in general, e.g.

from .main import ( PullReqState, parse_commands, db_query, INTERRUPTED_BY_HOMU_RE, synchronize, )

They take up more vertical space but I find them easier to read and cause much less diff noise, which is valuable for git log, git blame and friends.

This also works for method calls, dictionary and list literals, etc. in addition to imports.

Thanks for the feedback. I'll try a unilateral switch to block-style indent to see if anything goes wrong -- I feel like we have a few spots where it'll be adding unduly many lines (like https://github.com/servo/homu/pull/103/files#diff-7a3e9e91f0de1349c2561ed96ae5e206R511) but on the whole it's probably worthwhile for consistency.

OK, I just remembered the spot where block style indents fall apart: long function declarations.

Before:

def parse_commands(body, username, repo_cfg, state, my_username, db, states, *, realtime=False, sha=''): global global_cfg state_changed = False

Just sticking in a line break:

def parse_commands(body, username, repo_cfg, state, my_username, db, states, *, realtime=False, sha=''): global global_cfg state_changed = False

Full block-style indent needs the end paren with no indent, which looks really funny to me:

def parse_commands( body, username, repo_cfg, state, my_username, db, states, *, realtime=False, sha='' ): global global_cfg state_changed = False

Adding only a line break means you get unnecessary diff noise if you change the function name. I've gotten used to block indent so the ): without an indent looks fine to me, but I also think that there are further improvements to be had in the future by introducing some intermediate data structures as appropriate and reducing the general spaghetti-ness in further refactors.

Do you think this is worth going back through and tidying right now, or shall we land these fixups as-is and catch the diff noise reduction cleanups in future refactors?

aneeshusa · 2017-03-20T04:55:53Z

homu/server.py

@@ -39,7 +40,8 @@ def find_state(sha):
 def get_repo(repo_label, repo_cfg):
    repo = g.repos[repo_label].gh
    if not repo:
-        g.repos[repo_label] = repo = g.gh.repository(repo_cfg['owner'], repo_cfg['name'])
+        g.repos[repo_label] = repo = g.gh.repository(repo_cfg['owner'],


I find it clearer to put each assignment on its own line.

Whoops, this double assignment thingy bugged me too the first time I looked at it and then I split it totally wrong the second time. Thanks for pointing that out!

aneeshusa · 2017-03-20T04:59:11Z

homu/server.py

@@ -242,7 +255,7 @@ def github():
    payload = request.body.read()
    info = request.json

-    lazy_debug(logger, lambda: 'info: {}'.format(utils.remove_url_keys_from_json(info)))
+    lazy_debug(logger, lambda: 'info: {}'.format(utils.remove_url_keys_from_json(info)))  # NOQA


Since we're adding the #noqa comments, can you update the Travis file to prevent regressions by adding flake8 homu/server.py?

Also, I think #noqa works instead of #NOQA, which I find easier on the eyes (less shouty).

Good catch on the # noqa; I'll change those and use the more-legible variant in the future.

I'm doing basically the same thing to the rest of Homu this week, so that https://github.com/servo/homu/blob/master/.travis.yml#L10 in the Travis config can lose its --ignore E501.

I stuffed #noqa in quite liberally where I'm not sure of the consistent or best way to breeak things up. Refactored out the constant calls to verify_auth because they were painfully redundant. I *think* we're universally calling git_cmd with the same first args, and also github_create_status could just take state instead of state.get_repo and state.head_sha, but I'll have to double check both of those in all their uses to be sure.

edunham · 2017-03-23T04:48:25Z

This is like 80% of the way to better, and I shook out a few redundant function calls in the process.

I'd advocate for landing these changes promptly and ironing out the remaining inconsistencies during future refactors, because if I let this thing bitrot it'll be a real nightmare to fix up. Additionally I've allocated enough time this week to fix the damage that this prettiness refactor is doing to the handful of currently open PRs.

@Manishearth, @aneeshusa, thoughts?

larsbergstrom · 2017-03-24T19:12:38Z

@bors-servo r=larsbergstrom,aneeshusa

bors-servo · 2017-03-24T19:12:39Z

📌 Commit 6868caf has been approved by larsbergstrom,aneeshusa

bors-servo · 2017-03-24T19:12:42Z

⌛ Testing commit 6868caf with merge 8d56510...

@larsbergstrom

Begin pep8 fixups Partial pep8ening to get some feedback on the various styles of breaking up long lines that I've been using. @larsbergstrom, @Manishearth, could I get you to skim the diff and comment on any changes that you feel are a step in the wrong direction?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/homu/103)

bors-servo · 2017-03-24T19:13:37Z

☀️ Test successful - status-travis
Approved by: larsbergstrom,aneeshusa
Pushing 8d56510 to master...

aneeshusa reviewed Mar 20, 2017

View reviewed changes

edunham added 2 commits March 21, 2017 21:06

Begin pep8 fixups

377bed7

obvious fixups

ff4fc10

edunham force-pushed the pep8 branch from a5cfb0e to ff4fc10 Compare March 22, 2017 04:11

edunham added 4 commits March 21, 2017 21:11

type noqa prettier

a56a69c

pep8 utils

1be4acb

Flip actually-linting-stuff switch

6868caf

bors-servo merged commit 6868caf into servo:master Mar 24, 2017

This was referenced Mar 24, 2017

Support report deployment status by comment and Github Deployments #96

Closed

Fix PR template and Reviewable from merge commits #40

Open

Begin pep8 fixups #103

Begin pep8 fixups #103

Uh oh!

Conversation

edunham commented Mar 14, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsbergstrom commented Mar 18, 2017

Uh oh!

aneeshusa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edunham Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edunham commented Mar 23, 2017

Uh oh!

larsbergstrom commented Mar 24, 2017

Uh oh!

bors-servo commented Mar 24, 2017

Uh oh!

bors-servo commented Mar 24, 2017

Uh oh!

bors-servo commented Mar 24, 2017

Uh oh!

Uh oh!

edunham commented Mar 14, 2017 •

edited by larsbergstrom

Loading

edunham Mar 20, 2017 •

edited

Loading