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

lukpueh · 2020-06-30T08:55:38Z

--

Description of issue or feature request:

Following coding style guidelines is extremely important especially for the reference implementations we create at the lab. But I've become increasingly reluctant to point contributors to our guideline document, due to the issues outlined below.

Given that curating our own code style guide is a time-consuming task and there are many excellent code guides out there, I wonder if it wouldn't be better to restrict our guide to:

pointers to PEP 8 and other well-curated code style guides that match our expectations, and
only document the differences and short-comings we identify in those guides.

E.g. "Do as in PEP 8, but indent with 2 instead of 4 spaces!" (for reasons XYZ ... needs less horizontal space, consistency with existing code don't want a global flag day, ...)

I'd also like to say that I find the Google Python guide really good and it covers most of the principles of our guide in a clearer way, and other things that I find helpful.

Regardless, we should also configure strong linters on all projects (see e.g.
secure-systems-lab/securesystemslib#243) to enforce the code style we expect.

Issues with the current guideline

unclear mix of " and ' (see Single quotes (') or double quotes (") #4)
Python2-centered, e.g.: use of print statement and file built-in, mention of Python 2.3 (!!) and 2.6, no mention of Python 3
several inconsistencies between recommendations and examples, especially in the "Maximum line length" section, where the "how it should be done"-example has wrong line continuation indentation, unclear line break condition (neither hanging nor aligned indent, nor breaking close to 80 chars), no blank lines to separate logic, use of format operator which is advised against. Other inconsistencies include recommended but missing underscores in "Naming" section, missing whitespaces around operators in some string concat examples, etc...
some code examples are unnecessarily comprehensive, so that the emphasis on the specific recommendation suffers (especially in "Maximum line length", or in "Never use short circuit..")
syntax errors, e.g. assert(x = y), or the incorrectly continued strings in "Never use short circuit.."
obsolete recommendations about subprocesses (these things are documented in in the Python reference of the subprocess module and only add clutter here) and pointers to archived Seattle code
important drawbacks in our custom docstring format
1. not Sphinx-friendly
2. (less important) needs more vertical space than necessary
I suggest using Google style docstring, for which there is Sphinx extension, and which needs less vertical space than the similar NumPy style, and which (just as NumPy) is also friendlier to the human eye than the standard reStructuredText Docstring Format.

See Revise and update source code documentation and build with sphinx in-toto/in-toto#369 for a recent switch to Google style docstring.

The text was updated successfully, but these errors were encountered:

joshuagl · 2020-06-30T09:22:07Z

I'm really supportive of this.

Ideally we would be able to provide potential contributors a linter and editor configuration such that they don't need to think too hard about the coding style (and have the CI take care of verifying it for the maintainers). Currently it's hard for even well-intentioned contributors to meet the coding style because modern editors default to linting with an existing standard, i.e. VSCode enables linting by default with Pylint which follows PEP 8's style guide.

joshuagl · 2020-06-30T10:19:58Z

Note: the global flag day argument for sticking with 4-space indent is somewhat ameliorated by the --ignore-revs-file/blame.ignoreRevsFile option added in git 2.23. See a discussion on how to do that here: https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame

woodruffw · 2020-06-30T13:49:18Z

I suggest using Google style docstring, for which there is Sphinx extension, and which needs less vertical space than the similar NumPy style, and which (just as NumPy) is also friendlier to the human eye than the standard reStructuredText Docstring Format.

👍 to this! The Google docstring style is much easier to read when reviewing source, and tools like pdoc support it out of the box.

JustinCappos · 2020-06-30T14:42:41Z

I'm also supportive of this.

We should take a quick look / poll of other Python projects in the lab to be sure this won't cause a major issue first...

shibumi · 2020-07-29T13:17:34Z

Is the maximum line length of 79 characters per line set? I think the character limit of 79 is pretty oudated and even Linus Torvalds realized this and they are discussing increasing the limit for the kernel code right now.

Python can get very verbose (especially if you have lots of nesting). This gave me headache, while solving this PR here: in-toto/in-toto#379

The problem was, that we do not from securesystemslib import *, but on the same time we need to satisfy the line limit of 79 characters. This lead to this fix here:

https://github.com/in-toto/in-toto/blob/59058a886cce7e57e8b9660143f5bb81af016eda/in_toto/verifylib.py#L145L147

I needed to manually disable a specific python linter attribute to fix the issue related with the PR.
The linter attribute said:"variable has to follow snake_case", even if it's just a single letter.

lukpueh · 2020-08-04T10:20:01Z

Is the maximum line length of 79 characters per line set? I think the character limit of 79 is pretty oudated and even Linus Torvalds realized this and they are discussing increasing the limit for the kernel code right now.

I still think 79 is quite reasonable, especially when you only have two spaces per indentation level, as we do. :P It also allows me to have two pages of code side by side on my 13 inch laptop.

Python can get very verbose (especially if you have lots of nesting).

I think the solution here should be to avoid lots of nesting. :)

The problem was, that we do not from securesystemslib import *, but on the same time we need to satisfy the line limit of 79 characters.

Maybe we can use the google style import convention, I mentioned in this comment. This should lead to shorter lines.

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **Some more design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python2 **TODO:** See doc header TODO list

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **Additional design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python =< 3.5 **TODO: See doc header TODO list** Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Co-authored-by: Joshua Lock <jlock@vmware.com> Co-authored-by: Teodora Sechkova <tsechkova@vmware.com> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **TODO: See doc header TODO list** **Additional design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python =< 3.5 Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Co-authored-by: Joshua Lock <jlock@vmware.com> Co-authored-by: Teodora Sechkova <tsechkova@vmware.com> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

joshuagl · 2020-08-24T16:15:36Z

Maybe we can use the google style import convention, I mentioned in this comment. This should lead to shorter lines.

I like this suggestion. The required changes to module names will doubtless help any downstream's who use our libraries with this style of module import.

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **TODO: See doc header TODO list** **Additional design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python =< 3.5 Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Co-authored-by: Joshua Lock <jlock@vmware.com> Co-authored-by: Teodora Sechkova <tsechkova@vmware.com> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add convenience function to export multiple public keys from a gpg keyring into an sslib dict format at once and tests. Note: Uses the new pseudo-standard docstring style suggested in secure-systems-lab/code-style-guidelines#20. All new interface functions should use that style (existing docstrings will be converted in separate PRs).

Add convenience function to import multiple public keys of different key types from file into an sslib dict format at once, and tests. Note: Uses the new pseudo-standard docstring style suggested in secure-systems-lab/code-style-guidelines#20. All new interface functions should use that style (existing docstrings will be converted in separate PRs).

Add convenience function to export multiple public keys from a gpg keyring into an sslib dict format at once and tests. Note: Uses the new pseudo-standard docstring style suggested in secure-systems-lab/code-style-guidelines#20. All new interface functions should use that style (existing docstrings will be converted in separate PRs). Co-authored-by: Joshua Lock <jlock@vmware.com>

- Change docstring format to Google Style better auto-docs rendering (http://secure-systems-lab/code-style-guidelines#20) - Document recent interface function changes (args, errors) - Thoroughly document which exceptions may be raised - Correct mistakes about used encryption. - Generally overhaul docstrings with the goal to make them more concise, but more helpful for a user too, e.g. by mentioning signing schemes.

lukpueh · 2020-10-30T10:50:43Z

Something to consider for a note in a custom section about docstrings: theupdateframework/python-tuf#1193 (comment)

lukpueh · 2020-11-11T13:56:03Z

FYI, I'm in the process of writing a small diff/addition on top of the Google Python guide that I seems appropriate for our lab.

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-17T14:56:56Z

FYI, I'm in the process of writing a small diff/addition on top of the Google Python guide that I seems appropriate for our lab.

And here it is: #21

lukpueh mentioned this issue Jun 30, 2020

Consider not curating a full custom Python code-style guide in-toto/in-toto#370

Closed

adityasaky mentioned this issue Sep 1, 2020

What is secure-systems-lab/signing-spec? secure-systems-lab/dsse#1

Closed

lukpueh mentioned this issue Sep 30, 2020

Add gpg.export_pubkeys convenience function secure-systems-lab/securesystemslib#277

Merged

3 tasks

lukpueh mentioned this issue Sep 30, 2020

Add interface.export_pubkeys convenience function secure-systems-lab/securesystemslib#278

Merged

3 tasks

lukpueh mentioned this issue Oct 7, 2020

import-style prevents vendoring theupdateframework/python-tuf#1165

Closed

adityasaky mentioned this issue Oct 9, 2020

Merge snapshot module and create_layout module into one in-toto/layout-web-tool#49

Merged

This was referenced Oct 15, 2020

tuf/api: Expose tuf.api as a package (take 2) theupdateframework/python-tuf#1177

Merged

Revise pylint configuration (and linter choice) theupdateframework/python-tuf#1178

Closed

lukpueh mentioned this issue Oct 20, 2020

Format custom "Side Effects" docstring section in-toto/in-toto#401

Closed

lukpueh mentioned this issue Oct 30, 2020

Add root metadata class to new TUF metadata model theupdateframework/python-tuf#1193

Merged

3 tasks

lukpueh self-assigned this Nov 11, 2020

lukpueh mentioned this issue Nov 17, 2020

Replace styleguide with Google guide supplement #21

Merged

lukpueh closed this as completed in #21 Dec 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lukpueh commented Jun 30, 2020

joshuagl commented Jun 30, 2020

joshuagl commented Jun 30, 2020

woodruffw commented Jun 30, 2020

JustinCappos commented Jun 30, 2020

shibumi commented Jul 29, 2020 •

edited

Loading

lukpueh commented Aug 4, 2020

joshuagl commented Aug 24, 2020

lukpueh commented Oct 30, 2020

lukpueh commented Nov 11, 2020

lukpueh commented Nov 17, 2020

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

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

Comments

lukpueh commented Jun 30, 2020

joshuagl commented Jun 30, 2020

joshuagl commented Jun 30, 2020

woodruffw commented Jun 30, 2020

JustinCappos commented Jun 30, 2020

shibumi commented Jul 29, 2020 • edited Loading

lukpueh commented Aug 4, 2020

joshuagl commented Aug 24, 2020

lukpueh commented Oct 30, 2020

lukpueh commented Nov 11, 2020

lukpueh commented Nov 17, 2020

shibumi commented Jul 29, 2020 •

edited

Loading