Skip to content

Commit

Permalink
Add "British English" and "Import modules, not objects" rules
Browse files Browse the repository at this point in the history
Also a few other code review updates
  • Loading branch information
benhoyt committed Oct 3, 2024
1 parent d326572 commit a1ed48a
Showing 1 changed file with 52 additions and 6 deletions.
58 changes: 52 additions & 6 deletions STYLE.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@

# Ops Python style guide

This is the Python style guide we use for the Ops project. However, it's also the style we're converging on for other projects maintained by the Charm Tech team.

We use Ruff for formatting, and run our code through the Pyright type checker. We also try to follow [PEP 8](https://peps.python.org/pep-0008/), the official Python style guide. However, PEP 8 is fairly low-level, so in addition we've come up with the following style guidelines.

New code should follow these guidelines, unless there's a good reason not to. Sometimes existing code doesn't follow these rules, but we're happy for it to be updated to do so (either all at once or as nearby code is changed).

Of course, this is just a start! We add to this list as things come up in code review and we make a team decision.


Expand All @@ -17,7 +18,7 @@ Of course, this is just a start! We add to this list as things come up in code r

## Simplicity

### Avoid nested comprehensions
### Avoid nested comprehensions and generators

"Flat is better than nested."

Expand All @@ -27,7 +28,7 @@ Of course, this is just a start! We add to this list as things come up in code r
units = [units for app in model.apps for unit in app.units]

for current in (
status for status in pebble.ServiceStatus if status is not pebble.ServiceStatus.ACTIVE
status for status in pebble.ServiceStatus if status != pebble.ServiceStatus.ACTIVE
):
...
```
Expand All @@ -40,13 +41,51 @@ for app in model.apps:
for unit in app.units:
units.append(unit)

active_statuses = [status for status in pebble.ServiceStatus if status != pebble.ServiceStatus.ACTIVE]
for current in active_statuses:
for current in pebble.ServiceStatus:
if status == pebble.ServiceStatus.ACTIVE:
continue
...
```

## Specific decisions

### Import modules, not objects

"Namespaces are one honking great idea -- let's do more of those!"

When reading code, it's significantly easier to tell where a name came from if it is prefixed with the package name.

An exception is names from `typing` -- type annotations get too verbose if these all have to be prefixed with `typing.`.

**Don't:**

```python
from subprocess import run
from ops import CharmBase, PebbleReadyEvent
import typing

class MyCharm(CharmBase):
counts: typing.Optional[typing.Tuple[str, int]]

def _pebble_ready(self, event: PebbleReadyEvent):
run(['echo', 'foo'])
```

**Do:**

```python
import subprocess
import ops
from typing import Optional, Tuple

class MyCharm(ops.CharmBase):
counts: Optional[Tuple[str, int]]

def _pebble_ready(self, event: ops.PebbleReadyEvent):
run(['echo', 'foo'])
```


### Compare enum values by equality

This is six of one, half a dozen of the other, but we've decided that saying `if color == Color.RED` is nicer than `if color is Color.RED` as `==` is more typical for integer- and string-like values, and if you do use an `IntEnum` or `StrEnum` you should use `==` anyway.
Expand Down Expand Up @@ -76,6 +115,13 @@ if status != pebble.ServiceStatus.ACTIVE:

## Docs and docstrings

### Use British English

[Canonical's documentation style](https://docs.ubuntu.com/styleguide/en/) is to use British spellings, and we try to follow that here. For example, "colour" rather than "color", "labelled" rather than "labeled", "serialise" rather than "serialize", and so on.

It's a bit less clear when we're dealing with code and APIs, as those normally use US English, for example, `pytest.mark.parametrize`, and `color: #fff`.


### Spell out abbreviations

Prefer spelling out abbreviations and acronyms in docstrings, for example, "for example" rather than "e.g.", "that is" rather than "i.e.", and "unit testing" rather than UT.
Prefer spelling out abbreviations and acronyms in docstrings, for example, "for example" rather than "e.g.", "that is" rather than "i.e.", "and so on" rather than "etc", and "unit testing" rather than UT.

0 comments on commit a1ed48a

Please sign in to comment.