Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decide on style guideline #1128

Closed
lukpueh opened this issue Sep 10, 2020 · 15 comments · Fixed by #1232
Closed

Decide on style guideline #1128

lukpueh opened this issue Sep 10, 2020 · 15 comments · Fixed by #1232
Assignees
Labels
decision record Outcome of this discussion should be tracked in a decision record discussion Discussions related to the design, implementation and operation of the project documentation Documentation of the project as well as procedural documentation

Comments

@lukpueh
Copy link
Member

lukpueh commented Sep 10, 2020

There seems to be agreement to discontinue the Secure Systems Lab style guide and instead point to existing well-established guidelines (see secure-systems-lab/code-style-guidelines#20).

To get us started, I suggest to go with the Google Python style guide, and make decisions about acceptable/desirable deviations as they occur.

@lukpueh lukpueh added the discussion Discussions related to the design, implementation and operation of the project label Sep 10, 2020
@lukpueh
Copy link
Member Author

lukpueh commented Sep 10, 2020

One particular choice we have to make is, should we keep using our custom 2 space indentation or go for the more common 4 spaces.

We already use 4 spaces in #1112, which is sort of a foundation stone for the refactor. The main argument for 4 spaces is PEP8 compliance and thus easier integration with Python tooling / editors, which should also lower the contribution threshold.

And let's also decide about line continuation, while we're at it: #1112 (comment)

@joshuagl joshuagl added the documentation Documentation of the project as well as procedural documentation label Sep 10, 2020
@lukpueh lukpueh added the decision record Outcome of this discussion should be tracked in a decision record label Sep 10, 2020
@trishankatdatadog
Copy link
Member

Might as well use something standard like whatever black uses by default

@woodruffw
Copy link
Contributor

Putting my 0.02c in: black + pylint (or another popular linter, like flake8) makes me happy as a downstream consumer of TUF 🙂

The one tweak that I frequently see is to bump black's line break from 88 to 100, which I think is reasonable.

(This applies to #1157, which is currently dead in the CI because it exposes the changes made in #1112).

@joshuagl
Copy link
Member

I'm very much in favour of the Google Python style guide.

On the topic of indentation, I like 4 spaces mostly because it's easier (it's the default for most tools because of PEP 8). The Google Python style guide has this to say about indentation of line continuation:

In cases of implied line continuation, you should align wrapped elements either vertically, as per the examples in the line length section; or using a hanging indent of 4 spaces, in which case there should be nothing after the open parenthesis or bracket on the first line.

I'm a big fan of aligning continued lines with the opening delimiter.

@lukpueh
Copy link
Member Author

lukpueh commented Oct 30, 2020

I'm a big fan of aligning continued lines with the opening delimiter.

May I ask why? :P

I have a slight preference for hanging indent with two levels of indentation, i.e. 8 spaces to distinguish from logical indentation of nested blocks. (IIRC the Google guide is a bit inconsistent in that regard?, need to check)

My preference is rooted in some (semi-serious) arguments against aligned indentation, i.e.:

  • long 1. lines push continued lines to the right edge
  • opening delimiters' indentation might coincide with the logical nested block indentation, making it hard to distinguish
  • a rename in the 1. line might require re-aligning the continued lines

That said, I'm willing to go either way, if others prefer aligned indentation too, or if there are good arguments against hanging indent. But we IMO should pick one and not allow both.

@trishankatdatadog
Copy link
Member

Can someone show the visual difference between hanging vs alternative indent?

@lukpueh
Copy link
Member Author

lukpueh commented Nov 2, 2020

Sure...

# Hanging indentation:
def frobnicate(     # No args on first line
        foo, bar):  # Double-indent continued lines ...
    ...             # ... to distinguish from logical indentation
# NOTE: Google style guide does not seem to double-indent, but PEP8 does (see links below)



# Aligned indentation:
def frobnicate(foo,
               bar): # Align subsequent lines with opening delimiter
    ...

See
https://www.python.org/dev/peps/pep-0008/#indentation
https://google.github.io/styleguide/pyguide.html#34-indentation

@trishankatdatadog
Copy link
Member

OMG, aligned indentation, please, otherwise we'd have to stop being friends...

@joshuagl
Copy link
Member

joshuagl commented Nov 2, 2020

I'm a big fan of aligning continued lines with the opening delimiter.

May I ask why? :P

Familiarity, mostly. I prefer how it looks, but it's the norm in several ecosystems I've spent time writing code (and those ecosystems were languages that have explicit block delimiters).

  • opening delimiters' indentation might coincide with the logical nested block indentation, making it hard to distinguish

This is the argument I find most compelling, as code blocks in Python are whitespace delimited.

That said, I'm willing to go either way, if others prefer aligned indentation too, or if there are good arguments against hanging indent. But we IMO should pick one and not allow both.

Completely agree with this.

@mnm678
Copy link
Contributor

mnm678 commented Nov 2, 2020

Aligned indentation can lead to long lines if function/variable names are long, so I have a slight preference for hanging indentation, but I'd be fine either way

@lukpueh
Copy link
Member Author

lukpueh commented Nov 3, 2020

Aligned indentation can lead to long lines if function/variable names are long, so I have a slight preference for hanging indentation, but I'd be fine either way

@mnm678, do you mean the problem where long 1. lines push continued lines to the right edge?

def this_function_has_a_very_long_name_and_many_arguments(argument_1,
                                                          argument_2,
                                                          argument_3):
    """..."""


package.with.many.sub.packages.and.function_with_long_name(argument_1,
                                                           argument_2,
                                                           argument_3)

I think we can avoid this by advising against unreasonably long names and recommend an import convention à la:

import function_with_long_name from package.with.many.sub.packages.and

@lukpueh
Copy link
Member Author

lukpueh commented Nov 3, 2020

One thing I have been wondering about a few times and haven't seen any recommendations for is line continuation in continued lines, e.g. something like:

raise Exception("Lorem {} ipsum {} dolor sit amet, {} consectetur {} adipi {}"
                "rcitation {} ullamco".format(reprehenderit, voluptate velit,
                                              pariatur, deserunt, laborum)

Is this how others would indent the 2nd line of format arguments?

@trishankatdatadog
Copy link
Member

Is this how others would indent the 2nd line of format arguments?

YES

@mnm678
Copy link
Contributor

mnm678 commented Nov 3, 2020

@mnm678, do you mean the problem where long 1. lines push continued lines to the right edge?

Yes, but as you say, this can be prevented with reasonable naming conventions.

@joshuagl
Copy link
Member

joshuagl commented Nov 4, 2020

Is this how others would indent the 2nd line of format arguments?

That's what I would expect with aligned indentation, yes.

@lukpueh lukpueh self-assigned this Nov 11, 2020
lukpueh added a commit to lukpueh/tuf that referenced this issue Mar 12, 2021
Configure lint build in tox.ini to check if code in tuf/api/* is
formatted according to black and isort style rules:
https://black.readthedocs.io/en/stable/the_black_code_style.html
https://pycqa.github.io/isort/

In addition to our new style guide (theupdateframework#1128) and corresponding linter
configuration, requiring auto-formatting should help to further
reduce reviewing effort. The auto-formatter black was chosen for
the following reasons:
- It seems to be the most popular formatter in the Python ecosystem
- It is well documented including integration instructions with
  most of the tools we use (git, GitHub Actions, pylint, a range of
  editors, pyproject.toml theupdateframework#1161)
- It checks that the reformatted code produces a valid AST that is
  equivalent to the original
- It has almost no ways of customization, which means no
  customization effort required, and more (cross-project) style
  uniformity, lowering contribution barriers
- It converts single to double quotes, where reasonable, which is
  exactly what we recommend
- The style choices it makes seem generally reasonable and don't
 conflict with our style guide, except for favoring hanging over
 aligned indentation, which is the opposite of what we recommend.
 But we are willing to update the adapt our style guide.

Auto-format pre-commit configuration will be added in a subsequent
commit.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision record Outcome of this discussion should be tracked in a decision record discussion Discussions related to the design, implementation and operation of the project documentation Documentation of the project as well as procedural documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants