Make securedrop PEP8-compliant (mostly) and add PEP8 pre-commit hook #1471

ajvb · 2016-11-12T04:23:48Z

This PR is purely style fixes.

It makes the entire securedrop/ directory PEP8 compliant except for a few cases of E402 module level import not at top of file within the test/ directory.

This is because of overriding config.py like so:

import sys
...
# Set environment variable so config.py uses a test environment
os.environ['SECUREDROP_ENV'] = 'test'
import config

As well, this PR creates the git_helpers/ directory and adds a pep8 pre-commit hook.

One last thing, I would highly recommend adding CodeClimate (or similar) to do PEP8-on-PR checking per our conversation in #886. This will make sure that these style "problems" don't come back.

Example pep8 pre-commit hook output:

$ git commit
PEP8 style violations have been detected.  Please fix them
or force the commit with "git commit --no-verify".

./securedrop/source.py:26:80: E501 line too long (85 > 79 characters)

psivesely

Script looks good, and this is a useful hook I'd like to make available for developers.

I stopped commenting on it after the first few, and this is a person opinion, but I believe the syntax is too spaced out, sometimes for no gain in readability. Being able to see a large context of code is useful in my opinion, though I'm not just suggesting you cram as much as possible while being PEP8-compliant. Thoughts?

The one other thing is that something should go in the developer docs about this one.

psivesely · 2016-11-13T17:12:24Z

securedrop/db.py

@@ -109,7 +108,8 @@ def documents_messages_count(self):
            for submission in self.submissions:
                if submission.filename.endswith('msg.gpg'):
                    self.docs_msgs_count['messages'] += 1
-                elif submission.filename.endswith('doc.gz.gpg') or submission.filename.endswith('doc.zip.gpg'):
+                elif submission.filename.endswith('doc.gz.gpg') or \
+                        submission.filename.endswith('doc.zip.gpg'):


I'm not sure if the spacing on this bottom line is right. Further, I think we should follow PEP8 advice here:

The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation.

I also think think line breaks should come before binary operators (see https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator).

psivesely · 2016-11-13T17:14:45Z

securedrop/db.py

@@ -184,7 +184,8 @@ class SourceStar(Base):

    def __eq__(self, other):
        if isinstance(other, SourceStar):
-            return self.source_id == other.source_id and self.id == other.id and self.starred == other.starred
+            return self.source_id == other.source_id and \


Parens instead of backslash, break before binary operator.

psivesely · 2016-11-13T17:24:05Z

securedrop/journalist.py

+                        login_flashed_msg += (
+                            " Please wait for a new "
+                            "two-factor token before logging in again."
+                        )


Could probably be 2 lines.

psivesely · 2016-11-13T17:24:45Z

securedrop/journalist.py

+                        Journalist.MAX_PASSWORD_LEN
+                    ),
+                    "error"
+                )


In this one it's not just for compactness--I think the spacing you have here actually hurts readability.

psivesely · 2016-11-13T17:30:15Z

securedrop/db.py

@@ -131,7 +131,7 @@ class Submission(Base):
    source = relationship(
        "Source",
        backref=backref("submissions", order_by=id, cascade="delete")
-        )
+    )


Not a fan of this C-style syntax to be honest. I prefer to fit more content in a page view, so long as it doesn't hurt readability. IMO the right-hand side is not so complicated that putting it into two lines would hurt readability. I do use it here for example, where the two vars in the brackets would otherwise be flush with the rest of the arguments, making it otherwise harder to tell they were nested in a list.

ajvb · 2016-11-15T04:15:31Z

@fowlslegs Totally agree with the parenthesis over using backslashes, never saw that in PEP8.

In regards to the conversation that occurred in #886, what are your thoughts on:

Adding pep8 checking to Travis build in this PR?
Change "required line length" to something more reasonable like 100-120?

psivesely · 2016-11-15T18:54:31Z

I'd say yes. The general consensus at FPF seems to be we're in favor of making SD PEP8-compliant and enforcing that. One thing I just remembered is there is already a pylintrc file under securedrop/. You might want to look into modifying that and running pylint in Travis. I think it would be the way to go because it does a lot beyond PEP8. Not sure whether running that in Travis will make the results most accessible though, or what will be a failing condition as I've not used Pylint myself.
My personal vote is 99 based on a sample size of 2, and the fact I like odd numbers 乁໒( ͡◕ ᴥ ◕͡ )७ㄏ

psivesely · 2016-12-06T00:04:16Z

Okay, internal survey says FPF prefers 79/ official PEP8 decree.

psivesely · 2016-12-16T20:27:36Z

@ajvb is taking some time off, so someone should take over this PR. All that's left to be done is:

Rebase.
Fix in-line comments.
Add auto-checking to Travis.

conorsch · 2017-01-13T19:15:02Z

Ouch, badly in need of a rebase. Thank you for writing this, @ajvb! We'll get it over the finish line once the test churn settles down. @redshiftzero @fowlslegs is now a good time to tackle rebasing this?

conorsch · 2017-01-13T19:15:49Z

Ah, #1516 is still outstanding, let's get that in!

psivesely · 2017-03-02T23:21:18Z

Putting this in the 0.4 milestone issue because the associated issue, #886, currently is. As noted there, big changes in develop will require a messy rebase, and we need to discuss if this is something we want in 0.4 in general.

psivesely · 2017-06-26T19:42:48Z

I think we should either close this PR, or commit to reworking it for an upcoming release such as 0.4.1.

ajvb · 2017-06-27T22:08:22Z

@fowlslegs Agreed. Which would you advise?

redshiftzero · 2017-06-27T22:23:12Z

After 0.4 is released, let's rework this and get it into 0.4.1 (and maybe add flake8 to CI while we're at it)

psivesely · 2017-06-28T21:46:42Z

Testing story here is relevant #1894 (comment).

ajvb · 2017-06-29T03:55:27Z

@redshiftzero Sounds good!

redshiftzero · 2017-08-02T20:39:38Z

Following up now that 0.4 has been released. First, thanks again for your contributions @ajvb 💎. Unfortunately, as we can see from the conflicts, the codebase has changed significantly since this PR was written. However since then, @dachary has gradually flake8'd almost all the code in securedrop/ (just one last PR to go for that 🎉), and added flake8 checks to Travis CI.

However, I think the pre-commit hook in this PR would be really handy for developers. I've worked on a few projects that have flake8 in CI and it's pretty annoying to forget to flake8, push up code, wait for tests to pass, only to have it fail due to linting. @ajvb would you be interested in reworking this PR or opening a superseding PR to add a flake8 securedrop/ pre-commit hook?

Adding the int() to the string output from `wc -l` for integer comparison dragged the line over the 80 char mark, which caused the linting checks to fail on the testinfra directory. Fixed. Relevant to #1471, which included a pre-commit hook for linting that never got merged.

redshiftzero · 2017-09-01T22:26:26Z

Thanks again for your PR @ajvb. Unfortunately we are closing this PR in light of superseding PR #2237.

If you are interested in trying to contribute to SecureDrop again, please feel free - you're welcome to drop by our Gitter or take a look at anything tagged "hackathon" (these tend to be smaller issues).

ajvb · 2017-09-02T21:04:36Z

@redshiftzero Sounds good! My apologizes for being so unresponsive. Glad this was resolved nonetheless :)

AJ Bahnken added 3 commits November 11, 2016 19:38

First round of pep8 fixes

6ab60d8

PEP8 Fixes round two

f8caf8d

Create git_helpers and add pre-commit pep8 hook

85cc0cd

ajvb mentioned this pull request Nov 12, 2016

Automatically enforce PEP8 for Python code #886

Closed

psivesely suggested changes Nov 13, 2016

View reviewed changes

redshiftzero added the PR: Pending additional work label Dec 1, 2016

psivesely added source interface (SI) tests journalist interface (JI) labels Dec 16, 2016

psivesely assigned psivesely and unassigned psivesely Dec 16, 2016

psivesely added the help wanted! label Jan 13, 2017

psivesely added this to the 0.4 milestone Mar 2, 2017

psivesely removed this from the 0.4 milestone Mar 2, 2017

psivesely added journalist interface (JI) source interface (SI) tests labels Apr 6, 2017

psivesely added journalist interface (JI) source interface (SI) tests labels Apr 22, 2017

redshiftzero removed the help wanted! label May 10, 2017

redshiftzero added the PR: Targeted for future release label Jun 27, 2017

conorsch mentioned this pull request Aug 31, 2017

Provide pre-commit hook for developers #2234

Closed

heartsucker mentioned this pull request Aug 31, 2017

Added pre-commit hook #2237

Merged

1 task

redshiftzero closed this Sep 1, 2017

ajvb deleted the ajvb/pep8 branch September 2, 2017 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make securedrop PEP8-compliant (mostly) and add PEP8 pre-commit hook #1471

Make securedrop PEP8-compliant (mostly) and add PEP8 pre-commit hook #1471

ajvb commented Nov 12, 2016 •

edited

Loading

psivesely left a comment

psivesely Nov 13, 2016

psivesely Nov 13, 2016

psivesely Nov 13, 2016

psivesely Nov 13, 2016

psivesely Nov 13, 2016

ajvb commented Nov 15, 2016

psivesely commented Nov 15, 2016

psivesely commented Dec 6, 2016

psivesely commented Dec 16, 2016

conorsch commented Jan 13, 2017

conorsch commented Jan 13, 2017

psivesely commented Mar 2, 2017

psivesely commented Jun 26, 2017

ajvb commented Jun 27, 2017

redshiftzero commented Jun 27, 2017

psivesely commented Jun 28, 2017

ajvb commented Jun 29, 2017

redshiftzero commented Aug 2, 2017

redshiftzero commented Sep 1, 2017

ajvb commented Sep 2, 2017

Make securedrop PEP8-compliant (mostly) and add PEP8 pre-commit hook #1471

Make securedrop PEP8-compliant (mostly) and add PEP8 pre-commit hook #1471

Conversation

ajvb commented Nov 12, 2016 • edited Loading

psivesely left a comment

Choose a reason for hiding this comment

psivesely Nov 13, 2016

Choose a reason for hiding this comment

psivesely Nov 13, 2016

Choose a reason for hiding this comment

psivesely Nov 13, 2016

Choose a reason for hiding this comment

psivesely Nov 13, 2016

Choose a reason for hiding this comment

psivesely Nov 13, 2016

Choose a reason for hiding this comment

ajvb commented Nov 15, 2016

psivesely commented Nov 15, 2016

psivesely commented Dec 6, 2016

psivesely commented Dec 16, 2016

conorsch commented Jan 13, 2017

conorsch commented Jan 13, 2017

psivesely commented Mar 2, 2017

psivesely commented Jun 26, 2017

ajvb commented Jun 27, 2017

redshiftzero commented Jun 27, 2017

psivesely commented Jun 28, 2017

ajvb commented Jun 29, 2017

redshiftzero commented Aug 2, 2017

redshiftzero commented Sep 1, 2017

ajvb commented Sep 2, 2017

ajvb commented Nov 12, 2016 •

edited

Loading