Summary

Fix some PEP-257 issues in reviewboard/accounts/.

Review Request #6929 — Created Feb. 10, 2015 and submitted March 4, 2015, 1:25 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

f5fc853...

Reviewers

Groups

reviewboard

People

Description

If we're eventually going to integrate the PEP-257 tool into Review Bot (which

I think we should), we should get our docstring house in order first. This

change goes through reviewboard/accounts and fixes most of the issues that

aren't just a missing docstring.
The one "error" that I chose to disable for this is D203, which wants a blank

line before class docstrings. I didn't see any justification for this in the

actual PEP-257 document, and I think it's ugly. There are also a couple cases

that I chose to ignore where our test docstrings which wrap didn't follow the

usual rules. There are a ton of things that have missing docstrings, but those

will require more work to clean up.

Testing Done


Ran unit tests
Ran pep257 --ignore=D203 reviewboard/accounts

Issues

Description	From	Last Updated
The summary line should be on the """.	chipx86	Feb. 17, 2015, 2:56 p.m.
I briefly mentioned this in a different review request when this came up, but I think "Get" isn't explicit enough. …	chipx86	Feb. 17, 2015, 2:59 p.m.
Summary should be on the previous line.	chipx86	Feb. 17, 2015, 2:57 p.m.
Same here.	chipx86	Feb. 17, 2015, 2:57 p.m.
The old docs weren't very explicit, but they had more information than the new one. How about expanding to talk …	chipx86	Feb. 17, 2015, 3:01 p.m.
"Display" ?	chipx86	March 3, 2015, 1:49 p.m.
""" on the next line.	chipx86	March 3, 2015, 1:49 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/decorators.py
    reviewboard/accounts/backends.py
    reviewboard/accounts/views.py
    reviewboard/accounts/trophies.py
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/forms/auth.py
    reviewboard/accounts/forms/registration.py
    reviewboard/accounts/admin.py
    reviewboard/accounts/middleware.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/decorators.py
    reviewboard/accounts/backends.py
    reviewboard/accounts/views.py
    reviewboard/accounts/trophies.py
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/forms/auth.py
    reviewboard/accounts/forms/registration.py
    reviewboard/accounts/admin.py
    reviewboard/accounts/middleware.py

reviewboard/accounts/backends.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The summary line should be on the """.
```

reviewboard/accounts/backends.py (Diff revision 1)

The issue has been dropped. Show all issues

I briefly mentioned this in a different review request when this came up, but I think "Get" isn't explicit enough. Since we're describing it as an action, this sounds like it's saying that the function is getting the backends, but it's not clear that it's actually returning them. Lots of functions get things without returning them.

david Feb. 17, 2015, 3:01 p.m.

I'm not sure I agree. "Get" is definitely an overloaded term, but I think that when it's straightforward like this it makes sense (other uses of get would qualify it further, like "get and store X" or "Make an HTTP GET request")

chipx86 March 2, 2015, 11:44 p.m.

Hmm, but isn't this meant to describe what the function does, rather than what the caller does when calling it? The function doesn't get anything. It does return a thing, but it's not getting things.
A function like:

def foo()
    return 42


isn't getting the value 42. It's returning it.

reviewboard/accounts/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Summary should be on the previous line.
```
reviewboard/accounts/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/accounts/models.py (Diff revision 1)

Just curious, why is the blank line required? It doesn't seem to be consistent with the requirements for functions?

david Feb. 17, 2015, 3:01 p.m.

I think it's just following the rule that top-level items within a class are separated by a blank line.

reviewboard/accounts/views.py (Diff revision 1)

The issue has been resolved. Show all issues

The old docs weren't very explicit, but they had more information than the new one. How about expanding to talk about how the auth backend comes into play, and what happens if the auth backend doesn't support registration?

Commit:

-	68abf78a4a3f5e62a54409531fa6e41079267b7b
+	683a4816e79bb858434c60830c5062da788fcacf

Diff:

Revision 2 (+123 -84)

Show changes

	reviewboard/accounts/admin.py
	reviewboard/accounts/backends.py
	reviewboard/accounts/decorators.py
	reviewboard/accounts/managers.py
	reviewboard/accounts/middleware.py
	reviewboard/accounts/models.py
	reviewboard/accounts/pages.py
	reviewboard/accounts/tests.py
	5 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/decorators.py
    reviewboard/accounts/backends.py
    reviewboard/accounts/views.py
    reviewboard/accounts/trophies.py
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/forms/auth.py
    reviewboard/accounts/forms/registration.py
    reviewboard/accounts/admin.py
    reviewboard/accounts/middleware.py



Tool: Pyflakes
Processed Files:
    reviewboard/accounts/decorators.py
    reviewboard/accounts/backends.py
    reviewboard/accounts/views.py
    reviewboard/accounts/trophies.py
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/forms/auth.py
    reviewboard/accounts/forms/registration.py
    reviewboard/accounts/admin.py
    reviewboard/accounts/middleware.py

reviewboard/accounts/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"Display" ?
```
reviewboard/accounts/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
""" on the next line.
```

Commit:

-	683a4816e79bb858434c60830c5062da788fcacf
+	f5fc85325c78d225d68634366da02a8ac7f4edc5

Diff:

Revision 3 (+123 -83)

Show changes

	reviewboard/accounts/admin.py
	reviewboard/accounts/backends.py
	reviewboard/accounts/decorators.py
	reviewboard/accounts/managers.py
	reviewboard/accounts/middleware.py
	reviewboard/accounts/models.py
	reviewboard/accounts/pages.py
	reviewboard/accounts/tests.py
	5 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/decorators.py
    reviewboard/accounts/backends.py
    reviewboard/accounts/views.py
    reviewboard/accounts/trophies.py
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/forms/auth.py
    reviewboard/accounts/forms/registration.py
    reviewboard/accounts/admin.py
    reviewboard/accounts/middleware.py



Tool: Pyflakes
Processed Files:
    reviewboard/accounts/decorators.py
    reviewboard/accounts/backends.py
    reviewboard/accounts/views.py
    reviewboard/accounts/trophies.py
    reviewboard/accounts/pages.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/forms/auth.py
    reviewboard/accounts/forms/registration.py
    reviewboard/accounts/admin.py
    reviewboard/accounts/middleware.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (c40d908)