Replace styleguide with Google guide supplement #21

lukpueh · 2020-11-17T14:56:20Z

Closes #20, closes #3, closes #5 and closes #4

As discussed in #20 Secure Systems Lab does not need to maintain a custom Python Style Guide, given the availability of public style guides that too are based on PEP 8, but are more comprehensive and generally better curated.

This PR replaces the existing guidelines with a pointer to the Google Python Style Guide, which seems a good fit for us, and makes a few refinements, additions and accentuations.

This PR also moves the Python specific recommendations to a separate document, so that the README can be used as landing page for other guidelines in the future. Only the preamble of the README is kept and updated.

Most of the rules and recommendations from the old guide are covered in the new guide in a similar fashion. However, some items were changed or completely removed for below reasons:

List of changes/removals

'Nesting' is moved to 'Recommendations->Miscellaneous' and slightly updated to mention the "return early" pattern, which is what it is, and the "arrowhead anti-pattern", which is what it can help to avoid.
'Docstrings and comments' are covered in detail in the Google guide and refined in the new document, suggesting a more Sphinx-friendly docstring format and preserving some recommendations from the old guide.
Some recommendations about comments are removed, including "comments should document changes" (commit messages seem better suited), "write comment as a question" (seems a matter of taste) and "document your assumption" (seems like an excuse to not test you assumption, at least in the example).
"string formatting" is updated to recommend new style format() or newer style f-string syntax and mention pitfalls about string concatenation
The example in "Use else statements for handling errors, not for normal flow in most cases" conflicts with the "return early" recommendation (see above)
"Unix not Windows style text files" seems obsolete
"Avoid objects" and no "No lambda functions or lisp-esque code" seems too strict as a general rule. Similar more nuanced advice can be found in the Google guide, i.e. to avoid power features and that comprehensions, generator expressions and lamdba functions are okay for simple cases and one-liners.
The example in "Never use short circuit evaluation to produce output" is very confusing
Recommendations about 'os.popen', 'os.system', 'subprocess.Popen', are covered in the Python standard library reference
"Avoid changing directory" seems like a corner case

As discussed in secure-systems-lab#20 Secure Systems Lab does not need to maintain a custom Python Style Guide, given the availability of public style guides that too are based on PEP 8, but are more comprehensive and generally better curated. This commit replaces the existing guidelines with a pointer to the Google Python Style Guide, which seems a good fit for us, and makes a few refinements, additions and accentuations. This commit also moves the Python specific recommendations to a separate document, so that the README can be used as landing page for other guidelines in the future. Only the preamble of the README is kept and updated. Most of the rules and recommendations from the old guide are covered in the new guide in a similar fashion. However, some items were changed or completely removed for below reasons. **List of changes/removals** - 'Nesting' is moved to 'Recommendations->Miscellaneous' and slightly updated to mention the "return early" pattern, which is what it is, and the "arrowhead anti-pattern", which is what it can help to avoid. - 'Docstrings and comments' are covered in detail in the Google guide and refined in the new document, suggesting a more Sphinx-friendly docstring format and preserving some recommendations from the old guide. - Some recommendations about comments are removed, including "comments should document changes" (commit messages seem better suited), "write comment as a question" (seems a matter of taste) and "document your assumption" (seems like an excuse to not test you assumption, at least in the example). - "string formatting" is updated to recommend new style `format()` or newer style f-string syntax and mention pitfalls about string concatenation - The example in "Use else statements for handling errors, not for normal flow in most cases" conflicts with the "return early" recommendation (see above) - "Unix not Windows style text files" seems obsolete - "Avoid objects" and no "No lambda functions or lisp-esque code" seems too strict as a general rule. Similar more nuanced advice can be found in the Google guide, i.e. to avoid power features and that comprehensions, generator expressions and lamdba functions are okay for simple cases and one-liners. - The example in "Never use short circuit evaluation to produce output" is very confusing - Recommendations about 'os.popen', 'os.system', 'subprocess.Popen', are covered in the Python standard library reference - "Avoid changing directory" seems like a corner case Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh · 2020-11-17T15:14:00Z

@jhdalek55, would you mind checking this for language? It's probably nicer to read the updated README.md and new python.md, than reading the diff.

@JustinCappos, would you mind checking if the new text in README.md seems accurate to you?

jhdalek55 · 2020-11-17T15:41:37Z

Lukas, I'd be happy to look this over. Is there a set timeline for this? Can it wait just a few days? If its urgent, I can reshuffle things. Otherwise, I'll try to look it over before the end of the week.

Thanks.

Lois

lukpueh · 2020-11-17T15:51:32Z

Thanks, Lois! There is no rush. Later this week is definitely fine.

lukpueh · 2020-11-18T12:52:21Z

README.md

-```
-
-
-Use `else` statements for handling errors, not for normal flow in most cases.


I am a bit unsure about this recommendation and whether we should keep it (it is marked for removal in this PR).

IMO the example (below) is not about style but about correct programming. Although one may argue that the recommendation helps to avoid such programming errors?

If we want to keep the recommendation I would make it about switch (i.e. if/elif/.../else statements in Python) only. Because if you only have if/else, I'd rather recommend the "return early" pattern.

Agreed that this doesn't really feel like a programming style recommendation. It could also be interpreted as contradictory with EAFP style programming.

I'm be in favour of removing, as is already done in this PR.

lukpueh · 2020-11-18T13:02:10Z

README.md

-with `__`) are up to your discretion.
-
-
-### Never use short circuit evaluation to produce output


I am a bit unsure about this recommendation and whether we should keep it (it is marked for removal in this PR).

I find the examples very confusing, apart from distracting syntax errors I have to guess what askokcancel does, even though its call requires four lines. And the "YES" examples suggest excessive nesting that does not seem necessary (also see #3).

Is the problem here that askokcancel produces output? Or that it blocks on stdin? Is short circuit evaluation generally discouraged?

Either way, this PR should close #3.

lukpueh · 2020-11-18T14:03:10Z

python.md

+Adding a parenthesized identifier in a `# TODO:`-comment is not necessary. The
+version control system can provide that information just as well.
+
+### Strings


IMO we should add a clear recommendation for either double or single quoted strings here. See #4 (comment).

Any preferences, anyone?

I like double quotes because of the prevalence of apostrophes, but either is fine.

I default to double quotes as well.

I agree with double quotes.

Thanks! I added a recommendation in 6837167.

lukpueh · 2020-11-18T14:40:00Z

Curious to hear feedback from other Secure Systems Lab Python coders... cc @mnm678, @adityasaky, @SantiagoTorres, @joshuagl, @sechkova, @jku, @MVrachev

python.md

mnm678 · 2020-11-18T17:12:00Z

python.md

+Adding a parenthesized identifier in a `# TODO:`-comment is not necessary. The
+version control system can provide that information just as well.
+
+### Strings


I like double quotes because of the prevalence of apostrophes, but either is fine.

mnm678 · 2020-11-18T17:15:59Z

python.md

+
+### Testing
+
+<!-- TODO: Should we add more tangible instructions? -->


This might be self-explanatory, but we could give advice about testing at least all paths through the function, etc

Probably add that the test execution order shouldn't matter.
Also, because most of the tests are unit tests, maybe it's worth mentioning that unit tests shouldn't be dependent on external service and if they do, then mocking could be a solution.

Thanks. I decided to just remove this section to keep this document minimal. Let's think of more actionable recommendations in theupdateframework/python-tuf#1129 (where I also referenced your suggestions here).

jku · 2020-11-19T10:26:06Z

LGTM. I think project (or even lab) specific style guides should be as short as humanly possible (so I would probably remove the two sections that are currently marked "TODO"): a statement like "Follow existing code conventions and PEP-8" would probably have at least as much effect on code quality as 99% of the project specific style guides out there.

I agree with everything stated in the intro section, think that Google style guide looks fine and have no disagreements with the specific recommendations.

MVrachev · 2020-11-19T15:21:20Z

python.md

+ # YES:
+ raise Exception("Lorem {0} ipsum {1} dolor sit amet, {2} consectetur {3} ad "
+ "{4} rcitatio {5}.".format(reprehenderit, voluptate, velit,
+ pariatur, deserunt, laborum))


I support the idea to align the continued lines with the opening bracket, but this seems this is the opposite of Google's line breaking rules, right?
https://google.github.io/styleguide/pyguide.html#3192-line-breaking

Just wanted to make sure I understand it correctly. :)

I don't think so, the first line breaking rule is "Try to follow the existing indentation rules.".

MVrachev · 2020-11-19T15:29:43Z

A couple of thoughts:

What do we prefer: to import whole modules or specific functions from a module?
In the Google style guide about blank lines they say:

One blank line between method definitions and between the class line and the first method. 
No blank line following a def line.

One blank line between method definitions seems little to me.

Also, no black line after function declaration doesn't seem right especially when there is a multiline
function declaration like this:

def my_method(
    self, other_arg: Optional[MyLongType]
) -> Dict[OtherLongType, MyLongType]:
call_func()

which given that we are going to use type annotations will be often. :)

When this style-guide change is merged, how we are going to proceed with existing code in TUF for example?
We know that for the new API we are going to use the new style-guide, but what about the older code?

MVrachev · 2020-11-20T11:54:29Z

Another thing which I liked in the Google style guide and probably is worth emphasizing in our doc as well is this:

You should not document exceptions that get raised if the API specified in the docstring is violated 
(because this would paradoxically make behavior under violation of the API part of the API).

from here: https://google.github.io/styleguide/pyguide.html#doc-function-raises

joshuagl

LGTM. Thanks for writing this up @lukpueh. I completely agree with @jku's assessment that we should keep this guide as minimal as possible. For even moderately experienced programmers, it should be possible to infer as much of the coding style as possible by reading code that adheres to the style. I think we achieve that with PEP 8 + Google's guide.

python.md

joshuagl · 2020-11-24T10:51:11Z

python.md

+ # YES:
+ raise Exception("Lorem {0} ipsum {1} dolor sit amet, {2} consectetur {3} ad "
+ "{4} rcitatio {5}.".format(reprehenderit, voluptate, velit,
+ pariatur, deserunt, laborum))


I don't think so, the first line breaking rule is "Try to follow the existing indentation rules.".

joshuagl · 2020-11-24T10:52:04Z

python.md

+Adding a parenthesized identifier in a `# TODO:`-comment is not necessary. The
+version control system can provide that information just as well.
+
+### Strings


I agree with double quotes.

joshuagl · 2020-11-24T10:55:58Z

README.md

-```
-
-
-Use `else` statements for handling errors, not for normal flow in most cases.


Agreed that this doesn't really feel like a programming style recommendation. It could also be interpreted as contradictory with EAFP style programming.

I'm be in favour of removing, as is already done in this PR.

joshuagl · 2020-11-24T11:33:16Z

python.md

+
+ ```python
+ # NO:
+ import package.has.many.sub.packages.or_long_package_names.foo


This import style is contrary to style recommended by the Google guide. Is this example necessary?

AFAICS, the Google guide permits this one as well (see "verbose" vs. "common" import style), so I thought it might be good to advise against it here.

oh, so it does. Ugh. Then yes please let's advise against.

joshuagl · 2020-11-24T11:40:05Z

A couple of thoughts:

What do we prefer: to import whole modules or specific functions from a module?

The Google style guide says "Use import statements for packages and modules only, not for individual classes or functions.". I tend to agree, importing individual classes and functions is fiddly to maintain.

In the Google style guide about blank lines they say:
One blank line between method definitions and between the class line and the first method. 
No blank line following a def line. 
One blank line between method definitions seems little to me.

Also, no black line after function declaration doesn't seem right especially when there is a multiline
function declaration like this:
def my_method(
    self, other_arg: Optional[MyLongType]
) -> Dict[OtherLongType, MyLongType]:
call_func()

Python relies on indentation to delineate code blocks. In both of the examples there should be an indentation change. The Google style's use of relatively few blank lines works in conjunction with 4 spaces to indent. With two space indentations an indented block can be harder to spot, and a blank line can be a useful visual cue.

which given that we are going to use type annotations will be often. :)

When this style-guide change is merged, how we are going to proceed with existing code in TUF for example?
We know that for the new API we are going to use the new style-guide, but what about the older code?

Good question. I think for tuf we anticipate re-writing, or at least touching, most of the code during our refactor efforts. Therefore, I'd expect the code to match the style guide by the time 1.0 is released.

However, I'm not so sure for securesystemslib. I think it would make sense to ensure the code matches the style guide completely before we make a 1.0 release of that project, but I'm aware of the arguments against touching code for purely stylistic reasons, and against flag days.

Co-authored-by: Marina Moore <mnm678@users.noreply.github.com> Co-authored-by: Joshua Lock <jlock@vmware.com>

We want to keep the style guide as minimal as possible. Recommendations about code testing should go into a separate document (see theupdateframework/python-tuf#1129).

We want to keep the style guide as minimal as possible. A separate miscellaneous programming recommendations document might be added in the future.

lukpueh · 2020-12-01T11:46:35Z

Thanks for the many great comments! I just added a small note about string quotes, and removed testing and programming recommendations. To follow @jku's and @joshuagl's advice of keeping this document as short as possible.

The removed sections might resurface as part of other guidelines (see https://github.com/secure-systems-lab/code-style-guidelines/issues and theupdateframework/python-tuf#1129).

I'll merge this as is to move forward. I would, however, still appreciate feedback from @jhdalek55. I can address any comments in follow-up PRs. @jhdalek55, the documents can be found at:
https://github.com/secure-systems-lab/code-style-guidelines/blob/master/README.md, and
https://github.com/secure-systems-lab/code-style-guidelines/blob/master/python.md

lukpueh · 2020-12-01T11:50:29Z

@MVrachev, I hope @joshuagl's comments, which I agree with, answered your questions? Regarding the documentation of Exceptions in docstrings, I also found that piece of information in the Google guide quite interesting, but I don't think we need to replicate it in our guide, given that we say:

Follow the recommendations of the Google Python Style Guide for everything that is not covered in this document.

jhdalek55 · 2020-12-01T12:31:45Z

@lukpueh --I haven't forgotten about this...just been tied up with other work. I also like to wait till most content issues are resolved. I should be able to take a look today.

lukpueh mentioned this pull request Nov 17, 2020

Consider not curating a full custom Python code-style guide #20

Closed

lukpueh commented Nov 18, 2020

View reviewed changes

This was referenced Nov 18, 2020

Arrowhead anti-pattern in examples #3

Closed

Avoid trailing whitespaces #5

Closed

Single quotes (') or double quotes (") #4

Closed

lukpueh commented Nov 18, 2020

View reviewed changes

mnm678 reviewed Nov 18, 2020

View reviewed changes

MVrachev reviewed Nov 19, 2020

View reviewed changes

joshuagl approved these changes Nov 24, 2020

View reviewed changes

adityasaky mentioned this pull request Nov 25, 2020

Add grafeas in-toto interface in-toto/totoify-grafeas#3

Merged

lukpueh mentioned this pull request Dec 1, 2020

Misc programming recommendations #23

Open

lukpueh force-pushed the new-python-guidelines branch from 3a4f67e to 6ea78bc Compare December 1, 2020 10:20

lukpueh mentioned this pull request Dec 1, 2020

Add test guidelines theupdateframework/python-tuf#1129

Open

lukpueh and others added 4 commits December 1, 2020 12:33

Fix typos and minor reword in style guide

edf215c

Co-authored-by: Marina Moore <mnm678@users.noreply.github.com> Co-authored-by: Joshua Lock <jlock@vmware.com>

Add string quote type recommendation to style guide

f88d6c0

Remove testing recommendations

e8f1e72

We want to keep the style guide as minimal as possible. Recommendations about code testing should go into a separate document (see theupdateframework/python-tuf#1129).

Remove misc python programming recommendations

a0b32a9

We want to keep the style guide as minimal as possible. A separate miscellaneous programming recommendations document might be added in the future.

lukpueh force-pushed the new-python-guidelines branch from 6ea78bc to a0b32a9 Compare December 1, 2020 11:35

lukpueh merged commit 3fc216f into secure-systems-lab:master Dec 1, 2020

lukpueh mentioned this pull request Jan 15, 2021

Vendoring-compatible imports theupdateframework/python-tuf#1261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace styleguide with Google guide supplement #21

Replace styleguide with Google guide supplement #21

lukpueh commented Nov 17, 2020 •

edited

Loading

lukpueh commented Nov 17, 2020 •

edited

Loading

jhdalek55 commented Nov 17, 2020

lukpueh commented Nov 17, 2020

lukpueh Nov 18, 2020

joshuagl Nov 24, 2020

lukpueh Nov 18, 2020

lukpueh Nov 18, 2020

lukpueh Nov 18, 2020

lukpueh Nov 18, 2020

mnm678 Nov 18, 2020

adityasaky Nov 19, 2020

joshuagl Nov 24, 2020

lukpueh Dec 1, 2020

lukpueh commented Nov 18, 2020

mnm678 Nov 18, 2020

mnm678 Nov 18, 2020

MVrachev Nov 19, 2020 •

edited

Loading

lukpueh Dec 1, 2020

jku commented Nov 19, 2020

MVrachev Nov 19, 2020

joshuagl Nov 24, 2020

MVrachev commented Nov 19, 2020 •

edited

Loading

MVrachev commented Nov 20, 2020

joshuagl left a comment

joshuagl Nov 24, 2020

joshuagl Nov 24, 2020

joshuagl Nov 24, 2020

joshuagl Nov 24, 2020

lukpueh Dec 1, 2020

joshuagl Dec 1, 2020

joshuagl commented Nov 24, 2020

lukpueh commented Dec 1, 2020

lukpueh commented Dec 1, 2020

jhdalek55 commented Dec 1, 2020

		```


		Use `else` statements for handling errors, not for normal flow in most cases.

		with `__`) are up to your discretion.


		### Never use short circuit evaluation to produce output


		### Testing

		<!-- TODO: Should we add more tangible instructions? -->

Replace styleguide with Google guide supplement #21

Replace styleguide with Google guide supplement #21

Conversation

lukpueh commented Nov 17, 2020 • edited Loading

lukpueh commented Nov 17, 2020 • edited Loading

jhdalek55 commented Nov 17, 2020

lukpueh commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku commented Nov 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev commented Nov 19, 2020 • edited Loading

MVrachev commented Nov 20, 2020

joshuagl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuagl commented Nov 24, 2020

lukpueh commented Dec 1, 2020

lukpueh commented Dec 1, 2020

jhdalek55 commented Dec 1, 2020

lukpueh commented Nov 17, 2020 •

edited

Loading

lukpueh commented Nov 17, 2020 •

edited

Loading

MVrachev Nov 19, 2020 •

edited

Loading

MVrachev commented Nov 19, 2020 •

edited

Loading