README.md

Cookpad Python Style Guide

• 1 Background
  • 1.1 Conventions Precedence
• 2 Python Language Rules
  • 2.1 Python Version Support
  • 2.2 Imports
  • 2.3 Conditional expressions
  • 2.4 Type Annotations
  • 2.5 Asynchronous code
  • 2.6 Caching
• 3 Python Style Rules
  • 3.1 Line length
  • 3.2 Naming
  • 3.5 Interfaces
  • 3.6 Comments and docstrings
  • 3.7 Exceptions
  • 3.8 Classes
  • 3.9 Type access
• 4 Libraries
• 5 Tools
  • 5.1 pylint (static code analysis)
  • 5.2 pycodestyle (static code analysis)
  • 5.3 pydocstyle (static code analysis)
  • 5.4 black (automatic code formatting)
  • 5.5 pipenv (virtualenv management and dependency management)
  • 5.6 pytest (testing)
  • 5.7 mypy (static type checks)
  • 5.8 invoke (tasks automation)
• 6 How to Extend This Guide
  • 6.1 Editing tips

1 Background

Python is a dynamic language used at Cookpad. This style guide is a list of dos and don’ts for Python programs, inspired by the Google Python Style Guide. Rather than being a complete guide, this document aims to be a reference where decisions from previous Python style discussions are captured and to resolve ambiguities when they occur.

1.1 Conventions Precedence

On occasion, you may encounter a stylistic convention that is covered in a few references. These references may also disagree in their convention. When answering a question of style, please follow the following order of precedence:

1. Project specific conventions (where justified and absolutely necessary, and documented in a project-specific style guide)
2. Cookpad Python Style Guide (this document)
3. PEP8
4. Google Python Style Guide

Most ambiguities are resolved by using a code formatter and linting rules, detailed in subsequent sections. Avoid (1) if possible to allow consistent coding experience throughout the company. Prefer to update this document.

2 Python Language Rules

Language rules are code choices affecting the code behaviour. Some are enforced by pylint.

Code choices that don't have any effect on the code execution are documented in the section 3 Python Style Rules.

2.1 Python Version Support

The highest supported version is the latest stable 3.10 Python version.

Type of codebase	Version support
New project	highest, unless prevented by required dependencies
Existing project	the closest possible to highest
New/existing library	the closest possible to highest

2.2 Imports

• Use absolute imports.
• In case of a name conflict, import the module instead. If the package name conflicts as well, you may rename the import.
• Well known library abbreviations are allowed, if they are documented here.
• Do not use relative names in imports. Even if the module is in the same package, use the full package name. This helps prevent unintentionally importing a package twice.^G224
• Do not use star imports. They pollute the module name space and is very rare to want import of everything.

2.2.1 Do ✔️

• Use absolute imports:

  import module.sub_module
  from module.sub_module import SomeClass

• Import module on name conflict:

  from module import sub_module_a, sub_module_b
  
  print(sub_module_a.SomeClass)
  print(sub_module_b.SomeClass)

• Rename import on name and module conflict:

  from module.sub_a.module import SomeClass as SomeClassA
  from module.sub_b.module import SomeClass as SomeClassB

• Use well known library abbreviation:

  import dask.dataframe as dd
  import matplotlib as mpl
  import matplotlib.pyplot as plt
  import pandas as pd
  import seaborn as sns
  import tensorflow as tf
  import torch.nn.functional as F

2.2.2 Don't ✖️

• Don't use relative imports:

  from .module import sub_module
  from ..module import sub_sub_module

• Don't use star imports:

  from module.sub_module import *

• Don't use unnecessary renames:

  from module.sub_module import SomeClass as SomeOtherClass

2.3 Conditional expressions

Conditional expressions (sometimes called a “ternary operator”) are mechanisms that provide a shorter syntax for if statements.

Okay to use for simple cases. Each portion must fit on one line: true-expression, if-expression, else-expression. Use a complete if statement when things get more complicated.

2.3.1 Pros 👍

Shorter and more convenient than an if statement.

2.3.2 Cons 👎

May be harder to read than an if statement. The condition may be difficult to locate if the expression is long.

2.3.3 Do ✔️

one_line = 'yes' if predicate(value) else 'no'

slightly_split = (
    'yes' if some_long_module.some_long_predicate_function(value)
    else 'no, nein, nyet'
)
the_longest_ternary_style_that_can_be_done = (
    'yes, true, affirmative, confirmed, correct'
    if some_long_module.some_long_predicate_function(value)
    else 'no, false, negative, nay'
)

2.3.4 Don't ✖️

portion_too_long = (
    'yes' if some_long_module.some_long_predicate_function(
        really_long_variable_name
    )
    else 'no, false, negative, nay'
)

2.4 Type Annotations

You should annotate code with type hints according to PEP-484, and type-check the code with mypy. Because there is no need to support older Python versions that don't understand type annotations, all type annotations must be in source code, not in comments, docstrings or stub *.pyi files.^G221

Make sure you are familiar with types provided by the typing package and you use those, instead of built-in functions.

2.4.1 Pros 👍

• Type annotations improve the readability and maintainability of your code.
• The type checker will convert many runtime errors to build-time errors.
• Guides towards less complex code (no one wants to write complex annotations).

2.4.2 Cons 👎

You will have to keep the type declarations up to date. You might see type errors that you think are valid code.

2.4.3 Do ✔️

from typing import List, Dict

def func(a: int) -> List[int]:
    dictionary: Dict[str, bool] = {} 

2.4.4 Don't ✖️

• Don't use docstring type annotations:

  def func(a):
      """
      Args:
          a (int): Input value.
      Returns:
          List[int]: 10 instance of ``a``.
      """
      ...

• Don't use type annotations in comments:

  dictionary = {}  # type: Dict[str, bool] 

• Don't use built-in function instead of a type:

  def func(a: int) -> list[int]:
      dictionary: dict[str, bool] = {} 

2.4.5 Correct return type for iterators

• When annotating a generator that only yields but doesn't return or send(), use Iterator[...], not Generator[...]:

  from typing import Iterator
  def func() -> Iterator[int]:
      for i in range(10):
        yield i

2.4.6 Include | None for an argument that has a None default

If a function's argument defaults to None, it should include the | None type annotation. This is now the preferred approach in Python 3.10 and later.

2.4.6.1 Do ✔️:

def create_new_index(self, overide_index_timestamp: datetime | None = None) -> str:
    ...

2.4.6.2 Don't ✖️:

def create_new_index(self, overide_index_timestamp: datetime = None) -> str:
    ...

2.4.6.3 Don't ✖️:

def create_new_index(self, overide_index_timestamp: Optional[datetime] = None) -> str:
    ...

(Unless using Python <= 3.9)

2.5 Asynchronous code

Use async code only when you really need it.

• Async code is more prone to errors - forgetting await will often not raise any error in static code analysis because it is valid. It will return a Future instead of the expected result.
• Async code is more difficult to test.

If you use async, make sure you run tasks concurrently when possible. Using asyncio.gather() will cover most cases. There are other functions available if you need finer grained control.

2.5.1 Do ✔️

• Use asyncio.gather() to call multiple async functions concurrently:

  async def load_all(language_code: str, country_code: str):
      await asyncio.gather(
          load_definitions(language_code, country_code),
          load_stopwords(language_code),
          load_negators(language_code),
      )

• Use asyncio.gather(*(...)) to call a single async function in a loop concurrently:

  async def load_all(language_codes: List[str]):
      await asyncio.gather(
        *(load_definitions(language_code) for language_code in language_codes)
      )

2.5.2 Don't ✖️

• Don't use several awaits, unless you want them to be serialized.

  async def load_all(language_code: str, country_code: str):
      await load_definitions(language_code, country_code)
      # `load_stopwords` will start only when `load_definitions` has finished
      await load_stopwords(language_code)
      await load_negators(language_code)

  You may need the calls to be serialized if:

  • Calls are dependent (one uses results of another)
  • You want to limit the resource utilisation (such as not running 300 calls to a single server concurrently)

• Don't call await in a loop, unless you want the calls to be serialized:

  async def load_all(language_codes: List[str]):
      for language_code in language_codes:
          await load_definitions(language_code)
          # Next `language_code` will be loaded only when the previous one has finished 

• Don't use asyncio.wait(), unless you want a finer grained control over results/exceptions:

  async def load_all(language_code: str, country_code: str):
      # Any exceptions raised in the `load_*` functions below will be ignored
      await asyncio.wait(
          [
              load_definitions(language_code, country_code),
              load_stopwords(language_code),
              load_negators(language_code),
          ]
      )
      
  # The following code will re-raise exceptions, similar to `asyncio.gather`
  async def load_all_with_exceptions(language_code: str, country_code: str):
      done, pending = await asyncio.wait(
          [
              load_definitions(language_code, country_code),
              load_stopwords(language_code),
              load_negators(language_code),
          ]
      )
      assert not pending
      for task in done:
        task.result()

2.6 Caching

Use the appropriate caching technique as using a wrong one may lead to memory leaks.

Q: Should I use a permanent cache or a TTL cache?

In a live service (as opposed to a job), we should avoid cases where we cache something at service startup and then leave it cached indefinitely, except in cases where the underlying value(s) will certainly not change throughout the lifetime of the service.

✔️ Example where it's OK to permanently cache at startup:

• There are some static values held in a library that we load from disk. Those values only change when a new library version is released, so they'll be the same until the service is redeployed. We want to cache them to avoid repeatedly reloading them from disk during each request.

❌ Example where it's not OK to permanently cache at startup:

• Service pulls list of ingredients from search dictionary and loads them into memory. The reason for this is:
  • If the underlying data are changing, in most cases you probably want your feature to stay fresh (for some definition of "fresh")
  • You don't want big surprises when the service restarts. E.g., imagine the service is running for 5 days without restart, during which time the database has gone from 5 entries to 100000 entries.

Additional resources

• Careful when using lru_cache by Niko Wenselowski
• Python functools lru_cache with instance methods: release object, Stackoverflow
• functools.lru_cache keeps objects alive forever, Python 3.5 bug report

A summary of available alternatives at the time of writing:

library	function	method	dataclass method	async function	async method	TTL function	TTL method	TTL async function	TTL async method
functools	✔️	❌1	❌1	❌	❌	❌	❌	❌	❌
boltons.cacheutils	✔️	✔️	✅2	❌	❌	❌	❌	❌	❌
cachetools	✔️	✔️	✔️	✅3	✅3	✔️	✔️	✅3	✅3
async-cache	❌	❌	❌	✔️	❌	❌	❌	✔️	❌

2.6.1 Do ✔️

• Use cache for zero-parameter, synchronous functions, static or class methods (typically used for postponed evaluation or singletons):

  from functools import cache
  
  @cache
  def get_settings():
      ...
  
  class Singleton: 
      @cache
      @staticmethod
      def get() -> "Singleton":
          return Singleton()

• Use lru_cache with maxsize set on synchronous functions, static or class methods with parameters:

  from functools import lru_cache
  
  @lru_cache(maxsize=128)
  def multiply(a: int, b: int) -> int:
      return a * b
  
  @lru_cache(maxsize=1)
  def expensive_object_creation():
      ...

• Use cached_property on synchronous properties:

  from functools import cached_property  
  
  class Query:
      def __init__(self, value: str):
          self._value = value
    
      @cached_property
      def normalized(self) -> str:
          return self._value.strip().lower()

• Use cachetools and asyncache.cachedmethod on sync/async instance methods:

  from operator import attrgetter 
  
  from cachetools import LRUCache
  from asyncache import cachedmethod
  
  class Number:
      def __init__(self, value: int):
          self._value = value
          self._cache = LRUCache(max_size=128)
    
      @cachedmethod(attrgetter("_cache"))
      def multiply(self, other: int) -> int:
          return self._value * other
  
      @cachedmethod(attrgetter("_cache"))
      async def add(self, other: int) -> int:
          return self._value + other

  This works on dataclasses too:

  from typing import Awaitable
  from dataclasses import dataclass
  from operator import attrgetter
  
  from cachetools import cachedmethod, LRUCache 
  
  @dataclass(frozen=True)
  class Number:
      value: int
      _cache: LRUCache = LRUCache(max_size=128)
    
      @cachedmethod(attrgetter("_cache"))
      def multiply(self, other: int) -> int:
          return self._value * other
  
      @cachedmethod(attrgetter("_cache"))
      async def add(self, other: int) -> Awaitable[int]:
          return self._value + other

• Use cachetools and asyncache.cached for asynchronous functions or methods:

  from typing import Awaitable
  
  from asyncache import cached
  from cachetools import LRUCache
  
  @cached(LRUCache(128))
  def multiply(a: int, b: int) -> int:
      return a * b
  
  @cached(LRUCache(128))
  async def add(a: int, b: int) -> Awaitable[int]:
      return a + b

• Use cachetools if you need more advance caching, such as TTL:

  from cachetools import LRUCache, cached
  
  @cached(TTLCache(128, 3600))
  def multiply(a: int, b: int) -> int:
      return a * b
  
  from typing import Awaitable
  
  import asyncache
  
  @asyncache.cached(TTLCache(128, 3600))
  async def async_multiply(a: int, b: int) -> Awaitable[int]:
      return a * b

2.6.2 Don't ✖️

• Don't use lru_cache on instance methods. self is kept in the cache as a key, preventing the object to be garbage collected:

  from functools import lru_cache  
  
  class Query:
      def __init__(self, value: str):
          self._value = value
    
      @lru_cache(maxsize=1)
      def normalized(self) -> str:
          return self._value.strip().lower()
          
      @lru_cache(maxsize=1)
      def join(self, other: str) -> str:
          return self._value + other

• Don't use lru_cache on asynchronous function or methods. This decorator is not async compatible and the code will fail on subsequent executions:

  from functools import lru_cache  
  
  @lru_cache(maxsize=1)
  async def async_multiply(a: int, b: int) -> Awaitable[int]:
      return a * b

• Don't use unbound cache for any function/method that takes 1 or more parameters:

  from functools import lru_cache, cache  
  
  @lru_cache(maxsize=None)
  def multiply(a: int, b: int) -> int:
      return a * b
  
  @cache
  def multiply(a: int, b: int) -> int:
      return a * b

3 Python Style Rules

Style rules are (non-)code choices having no effect on code behaviour. They are enforced by pylint, pydocstyle, pycodestyle and black.

Code choices that do affect code execution are documented in the section 2 Python Language Rules.

3.1 Line length

Maximum line length is 120 characters. Compliance with this line length requirement is taken care of by black.

3.2 Naming

1. Function names, variable names, and file names should be descriptive; eschew abbreviation. In particular, do not use abbreviations that are ambiguous or unfamiliar to readers outside your project, and do not abbreviate by deleting letters within a word.^G316

2. Python file names must have a .py extension and must not contain dashes (-). This allows them to be imported and unit tested.

3. "Internal" means internal to a module, or protected or private within a class.

4. Prepending a single underscore (_) has some support for protecting module variables and functions (not included with from module import *). While prepending a double underscore (__ aka "dunder") to an instance variable or method effectively makes the variable or method private to its class (using name mangling) we discourage its use as it impacts readability and testability and isn’t really private. Lint warnings take care of invalid access to protected members.

5. Place related classes and top-level functions together in a module. Unlike Java, there is no need to limit yourself to one class per module. Unless you reach module size limit mandated by the linter.

6. Use class MyClass for class names, my_class.py for module names.

7. Use test_my_class.py for module related unit tests, class TestMyClass / class TestMethodOfMyClass for grouping related tests and def should_do_something for individual tests.

Type	Public	Internal
Packages	lower_with_under
Modules	lower_with_under.py	lower_with_under.py
Classes	CapWords	_CapWords
Exceptions	CapWords
Functions	lower_with_under()	_lower_with_under
Global/Class Constants	CAPS_WITH_UNDER	_CAPS_WITH_UNDER
Global/Class Variables	lower_with_under	_lower_with_under
Instance Variables	lower_with_under	_lower_with_under
Method Names	lower_with_under()	_lower_with_under()
Function/Method Parameters	lower_with_under
Local Variables	lower_with_under

3.2 Names to Avoid

1. 1-2 character names except for counters or iterators. You may use ex as an exception identifier in try/except statements and fd for file descriptor in with/open statements.
2. dashes (-) in any package/module name
3. __double_leading_and_trailing_underscore__ names (reserved by Python)
4. Package names identical to standard libraries. Example: test → tests
5. Package names identical to used external libraries. Example: elasticsearch → elastic_search

3.3 Spelling

For source code, use American spelling, not British spelling.

3.3.1 Do ✔️

class Serializer:
    color = 0x00ffffff

3.3.2 Don't ✖️

class Serialiser:
    colour = 0x00ffffff

3.4 Unused Function/Method Parameters

Unused argument can be marked as such by deleting the variables at the beginning of the function/method. Always include a comment explaining why you are deleting it. "Unused" is sufficient.

3.4.1 Do ✔️

def viking_cafe_order(spam, beans, eggs, haggis):
    del beans, eggs, haggis  # Unused by vikings.
    return spam + spam + spam

3.4.2 Don't ✖️

1. Don't use a prefix:

   # Breaks callers that pass arguments by name
   def viking_cafe_order(spam, _, unused_eggs, _haggis):
       return spam + spam + spam

2. Don't assign to _:

   # Does not enforce that the arguments are actually unused
   def viking_cafe_order(spam, beans, eggs, haggis):
       _ = beans, eggs, haggis
       return spam + spam + spam

3.5 Interfaces

Clearly communicate what is the interface of a class or module by appropriately marking entities as public or internal (see 3.2 Naming). As a rule of thumb, start with internal and make it public only if you need to.

If certain instance variables are read/write only, use properties or setters.

In case of data classes, consider making them immutable/frozen.

3.5.1 Pros 👍

• The intended use is clearer with less documentation needed.
• Less of unexpected side effects.

3.5.2 Do ✔️

_MODULE_CONSTANT = None

class _InternalHelperClass:
    _internal_class_variable = True
    public_class_variable = False


class PublicClass:
    def __init__(self):
        self._internal_instance_variable = _MODULE_CONSTANT

    @property
    def read_only_public_instance_variable(self):
        return self._internal_instance_variable

3.5.3 Don't ✖️

MODULE_CONSTANT = None  # Can be imported

class InternalHelperClass:  # Can be imported
    # internal_class_variable can be changed, not clear it should not
    internal_class_variable = True
    public_class_variable = False


class PublicClass:
    def __init__(self):
        # internal_instance_variable not read-only, not clear it should be
        self.internal_instance_variable = MODULE_CONSTANT

    @property
    def read_only_public_instance_variable(self):
        return self.internal_instance_variable

3.6 Comments and docstrings

Docstrings are formatted using the Google style (as opposed to reST or numpy), as this is the most readable format. See an example. This format is partially enforced by pydocstyle.

For docstring and comments format, please refer to the Google Python Style Guide, section 3.10. For documenting types, see 2.4 Type Annotations.

3.6.1 Notable conventions

• Use double backticks for code in docstrings:

  def method(value):
      """Modifies parameter ``value`` in place."""
      ...

• Use single backticks for code in comments:

  def method(value):
      value += " modified"  # Modifies parameter `value` in place

3.7 Exceptions

Exceptions should be full and grammatically correct sentences.

3.7.1 Do ✔️

raise ValueError(
    f"Unknown object type {obj.__class__.__name__} for value {obj}. "
    f"Consider adding support for it if acceptable."
)

raise NotImplementedError("Expected to be defined in sub-classed.")

3.7.2 Don't ✖️

raise ValueError(
    f"Unknown object type {obj.__class__.__name__} for value {obj}. "
    f"Consider adding support for it if acceptable"  # missing dot
)

raise NotImplementedError("expected to be defined in sub-classed")

3.8 Classes

If a class inherits from no other base classes, do not inherit from object. This also applies to nested classes.

Explanation

There is no need to inherit from object in Python 3. This was required in Python 2 to generate new-style classes. Python 3 doesn't have old-style classes anymore, thus there is no difference between classes inheriting from object and those which don't.

3.8.1 Do ✔️

class SampleClass:
    pass

class OuterClass:
    class InnerClass:
        pass

3.8.2 Don't ✖️

class SampleClass(object):
    pass

class OuterClass(object):
    class InnerClass(object):
        pass

3.9 Type access

Access class of an object obj via obj.__class__, not type(obj).

Explanation

There is no benefit in using type in new-style classes.

3.9.1 Pros 👍

• More readable than type(obj).

3.9.2 Cons 👎

• Slightly longer representation.

3.9.3 Do ✔️

def dynamic_function(value: str):
    if not isinstance(value, str):
        raise TypeError(f"Unexpected type '{value.__class__.__name__}' used for 'value'")

3.9.4 Don't ✖️

def dynamic_function(value: str):
    if not isinstance(value, str):
        raise TypeError(f"Unexpected type '{type(value).__name__}' used for 'value'")

3.10 Strings formatting

Prefer f-strings over str.format. Don't use % formatting.

There are a few legitimate uses of str.format:

• formatting dictionaries
• string templating

3.10.1 Do ✔️

f"The value is {value}."  # "The value is 80."
f"{date} was on a {date:%A}"  # "1991-10-12 was on a Saturday"
f"{{{4*10}}}"  # "{ 40 }"
f" [line {lineno:2d}]"
f"{extra},waiters:{len(self._waiters)}"

• Formatting a dictionary:

  @dataclass
  class Point:
      x: int
      y: int
      
      def __str__(self):
          return "X: {x}, Y: {y}".format(self.__dict__)

• String templating, template is dynamic:

  @dataclass
  class Point:
      x: int
      y: int
      
      def format(self, output_template: str):
          return output_template.format(self.__dict__)
  
  Point(1, 2).format("X: {x}, Y: {y}")

• String templating, template is used multiple times:

  query_template = "/recipes?query={query}&country_code=GB&provider_id=1"
  
  client.get(
      query_template.format(query=Faker().sentence()),
      name=query_template.format(query="<RANDOM SENTENCE>"),
  )

3.10.2 Don't ✖️

• Don't use % formatting:

  'error: %s' % msg

• Don't use unnecessary str.format:

  "The value is {value}.".format(value=123) 

3.11 Multi-line strings

In multi-line strings, use only regular strings or only f-strings. Combining them often leads to unpopulated placeholders.

3.11.1 Do ✔️

• Use regular strings if there is no string formatting needed:

  value = (
      "This is an extra long string "
      "wrapped to multiple lines."
  )

• Use f-strings for string formatting:

  value = (
      f"This is an extra long string with a {placeholder} "
      f"wrapped to {count} lines."
  )

• Use f-strings consistently, even when only some lines need to be formatted:

  value = (
      f"This is an extra long string with a {placeholder}. "
      f"This line has no placeholder but should consitently use an f-string."
  )

3.11.2 Don't ✖️

• Don't mix regular and f-strings:

  value = (
      f"This is an extra long string with a {placeholder} wrapped to "
      "{count} lines. I added the placeholder later and forgot to add the 'f'."
      # ^^^ Oops, this will render "{count} lines"
  )

4 Libraries

The following section describes recommended and commonly used libraries, including conventions for their use.

5 Tools

Following section describe recommended and commonly used tools, including conventions for their use.

5.1 pylint (static code analysis)

pylint is a tool for finding bugs and style problems in Python source code. It finds problems that are typically caught by a compiler for less dynamic languages like C and C++. Because of the dynamic nature of Python, some warnings may be incorrect; however, spurious warnings should be infrequent.

5.1.1 Pros 👍

Catches easy-to-miss errors like typos, using-vars-before-assignment, etc., therefore faster PRs with fewer comments.

5.1.2 Cons 👎

pylint is not perfect. To take advantage of it, we will need sometimes to:

1. write around it,
2. suppress its warnings,
3. or improve it.

5.1.3 Suppressing warnings

Before you start thinking about suppressing warnings, make sure you fully understand the error message and the issue can't be solved in code.

Suppress warnings if they are inappropriate so that other issues are not hidden. To suppress warnings, you use # pylint: disable=<RULE NAME>, in this order of preference:

• line-level: use for a single disable
• block-level: use if the same error appears multiple times in a single block or can't be easily disable on the same line
• file-level: suggests a larger issue where either the rules need to change or the solution needs a different direction.

Additionally:

• Use symbolic names to identify pylint warnings, not numeric codes.
• Always add an explanation for your disable above or below the suppression.

See also section 3.4 Unused Function/Method Parameters.

5.1.3.1 Pros 👍

• Suppressing in this way has the advantage that we can easily search for suppressions and revisit them.
• You may realise you don't need the suppression at all while trying to explain it in the comment (rubber ducking).

5.1.3.2 Do ✔️

# My explanation for this disable
dict = 'something awful'  # pylint: disable=redefined-builtin

5.1.3.3 Don't ✖️

• Don't use numeric code instead of symbolic:

  # My explanation for this disable
  dict = 'something awful'  # pylint: disable=W0622

  Not clear what is disabled.

• Don't leave out explaining comment:

  dict = 'something awful'  # pylint: disable=redefined-builtin

  Is this disable still needed? What was the motivation?

5.2 pycodestyle (static code analysis)

5.3 pydocstyle (static code analysis)

5.4 black (automatic code formatting)

Black is an opinionated Python code formatter.

5.4.1 Pros 👍

• Speed, determinism, and freedom from pycodestyle nagging about formatting.
• Saves time and mental energy for more important matters.
• Makes code reviews faster by producing the smallest diffs possible.
• Blackened code looks the same regardless of the project you’re reading.
• Formatting becomes transparent after a while and you can focus on the content instead.

5.4.2 Cons 👎

• Not everyone will agree with the formatting.
• Manual intervention needed for long strings (black will not wrap them).
• It doesn't and will not order imports. Consider using isort for that purpose.

5.4.3 Do ✔️

• Run black with pre-commit before every push. See the Tasks Automation for guidance on combining black with pre-commit.

• Check code into CI if it is formatted.

• Map black to a short cut in your IDE.

  Example for PyCharm

  Step 1: register external tool

  Step 2: register keyboard shortcut for running the external tool

5.4.4 Don't ✖️

• Don't use the magic trailing comma unless you want to communicate adding more items soon:

  # The parameters are not going to change any time soon
  ingredients=[
    {
      "name": "Turkey",
      "quantity": "1",
      "position": 0  # Do not use a comma here
    },
  ]

• Don't use the editor built-in formatter. While it may do a good job at formatting, it also may not be opinionated. Everyone could have different settings, resulting in different formatting. This difference will result in a lot of unnecessary changes in pull requests by hopping from one style to another.
• Don't use yapf. It is just less popular alternative.

5.5 pipenv (virtualenv management and dependency management)

Pipenv automatically creates and manages a virtualenv for projects, as well as adds/removes packages from a Pipfile as you install/uninstall packages. It also generates a Pipfile.lock, which is used to produce deterministic builds.

Q: Why not poetry?

pipenv was chosen when both pipenv and poetry were equals. Poetry has evolved since then to have better support. But there wasn't any reason good enough to migrate all project yet. See how they compare in star rating. A future migration could be made easier with dephell.

Q: How to fix versions of the "types-*" modular typeshed packages?

Mypy 0.900 brings modular typeshed packages. This can result in a lot of types-* packages in the Pipfile. While they all use semantic versioning, vast majority seem to be in the "initial development" (version 0.x). In semantic versioning, this means that any subsequent version can be breaking (for example 0.1.1 -> 0.1.2). In case of type packages, breaking means it will start failing type checks where it previously hasn't. This is solved by locking these dependencies to an exact version so that upgrade requires a manual edit of the Pipfile. However, doing this for a lot of packages without them actually breaking creates a maintenance overhead.

As a reasonable compromise for maintainability of these packages, set all types-* packages to * (the latest version), fix to major or exact version only if type check fails.

5.5.1 Pros 👍

• deterministic builds (as opposed to requirements.txt)

5.5.2 Cons 👎

• slow (as opposed to poetry, requirements.txt)
• maintenance is almost non-existent

5.5.3 Do ✔️

• Lock semantically versioned stable dependencies (version >= 1.0) to minor version with the PEP-440 compatible release operator:

  pytest = "~=5.4"
  # version 6 will have breaking changes, upgrade should be explicit
  # version 5.5 is not breaking, upgrade can be automatic
  

• Lock dependencies that are not semantically versioned or in initial development (version 0.x) to a specific version with the PEP-440 version matching operator:

  mypy = "==0.790"
  # any new version can be breaking, upgrade mustn't be automatic
  
  respx = "==0.15.0"
  # semantically versioned but any version can be breaking at this stage
  

• Leave development dependencies unlocked if breaking changes don't affect anybody:

  ipython = "*"
  # you often use ipython in the project but want the latest
  

• Leave typeshed packages unlocked, unless it keeps breaking type checks often. If it does, lock to the next least strict version that will prevent the breaking change:

  types-requests = "*"
  
  # version 6.x breaks type checks and we are not ready to move the main
  # package to version 6.x
  types-PyYAML = "~=5.4"
  

• Install packages in a reproducible way:

  pipenv install --deploy
  # Aborts if the Pipfile.lock is out-of-date, or Python version is wrong.

5.5.4 Don't ✖️

• Don't leave code or testing dependencies unlocked:

  pytest = "*"  # could unexpectedly start failing in CI pipeline
  requests = "*"  # could have API change and break the code
  

5.5.5 Good hygiene for third party packages

As an organisation, we embrace the use of third party open source packages in our software. Third party software comes with some risks and can be a vector for malicious software attacks. To mitigate risks we recommended following industry best practices.

This section is used to record recommendations for dealing with third party packages, although it is not yet complete. You should follow your own discretion (and industry best practices) when dealing with third party packages. All code authors and code reviewers should take particular care to evaluate new third party packages they are introducing, and also to vet version updates when bumping existing dependencies.

5.5.5.1 Do ✔️

• When updating existing third party dependencies, verify the provenance of each updated version. Use pipenv update --dry-run for a list of packages that can be updated. This is to avoid dependency hijack attacks.

• When referring to any package that is not hosted on the official PyPI (Python Package Index), you should explicitly refer to the source index from which the dependency will be imported, using {..., index="my-index-name"}. The following example is a Pipfile that is correctly adhering to this rule:

  [[source]]
  name = "pypi"
  url = "https://pypi.org/simple"
  verify_ssl = true
  
  [[source]]
  url = "http://pypi.home.kennethreitz.org/simple"
  verify_ssl = false
  name = "kennethreitz"
  
  # ...
  
  [packages]
  "your-favorite-lib" = {version="~=1.0", index="kennethreitz"}  # <-- refers to `kennethreitzth` from the source list

This will ensure that the package comes from the intended source, rather than a package from PyPI (or any other index on your list of sources) that happens to have the same name. This applies when referring to any non-PyPI index, including internal Cookpad packages hosted on our internal index. A project that fails to follow this rule is at risk of a dependency confusion attack.

5.6 pytest (testing)

The pytest framework makes it easy to write small tests yet scales to support complex functional testing for applications and libraries.

5.6.1 File System Structure

All tests are expected in a directory called tests. This is further subdivided:

• By type of tests:

  • unit for unit tests
  • integration for integration tests

• By type of code shared among tests of any type:

  • fixtures for pytest fixtures shared among different tests
  • utils for any extra helper code used exclusively by tests
  • fakers for code generating fake instances of objects, usually with faker

There should be no nested directories of the same names.

Additionally, all packages must contain an __init__.py file to prevent name clashes.

5.6.1.1 Pros 👍

• Avoids clash with test, which is a standard package.
• Predictable locations.
• Allows running tests easily by type or all at once.
• Discourages code duplication

5.6.2 Unit Tests Organisation

Directory structure under tests/unit replicates directory structure of the source package. Test file is the same as the file it tests, with a test_ prefix.

If the test file grows too large, it may be an indicator or either missing parametrization or the source module doing too much. Try to introduce parametrization or split the source code first.

5.6.2.1 Pros 👍

• Allows easier finding of relevant tests.

5.6.2.3 Do ✔️

For the following source package:

src/
    package/
        module.py

• Test file matching source file

  tests/
      unit/
          package/
              test_module.py
  

5.6.2.4 Don't ✖️

For the same source package as in Do's:

• Don't use mismatching path:

  tests/
      unit/
          test_module.py
  

• Don't leave out split by type:

  tests/
      module/
          test_module.py
  

• Don't use unpredictable file names:

  tests/
      unit/
          package/
              test_multiple_modules_in_one_file.py
  

5.6.3 Integration Tests Organisation

Unlike unit tests, integration tests don't relate to a specific file . Therefore, the directory structure under tests/integration should centre around features. All files must use the test_ prefix.

5.6.4 Test File Organisation

Always put a test in a class, even though pytest does not mandate it. Name the class after the thing you test. It can be a class, a feature, etc. The class must start with Test.

Start test cases with should_ rather than test_. The test case should require no docstring. All the information should fit in the method name.

The class name, method name and test parameters (if used) should for a sentence.

5.6.4.1 Pros 👍

• Readable test output - it makes sense without additional context.
• Guides towards small, focused tests because of thinking "I'm testing something. It should do something."
• Focuses on features rather than line coverage.

5.6.4.2 Cons 👎

• Tests often need to be static methods because self is not used.

5.6.4.3 Do ✔️

Test of function datetime_to_iso_zulu from utils.py in test_utils.py.

class TestDatetimeToIsoZulu:
    @staticmethod
    def should_convert_datetime_to_utc_string():
        assert ...

    @staticmethod
    @pytest.mark.parametrize(
        "instance", [
            pytest.param(datetime.now(), id="datetime without timezone"),
            pytest.param(None, id="None"),
        ]
    )
    def should_refuse():
        assert ...

This will generate tests:

• test_utils.TestDatetimeToIsoZulu.should_convert_datetime_to_utc_string
• test_utils.TestDatetimeToIsoZulu.should_refuse.datetime without timezone
• test_utils.TestDatetimeToIsoZulu.should_refuse.None

5.6.4.4 Don't ✖️

• Don't leave out a class:

  def test_convert_datetime_to_utc_string():
      assert ...

  Test of what?

• Don't name test cases poorly:

  def test_1():
      assert ...

  Can you tell what was supposed to happen if it fails?

5.6.5 Tests Parametrization

Prefer to parametrize test cases with pytest.mark.parametrize instead of copy and pasting them. Additionally, make sure each example has a readable ID by using pytest.param(..., id="…"). Default ones are rarely readable. See also section 5.6.4 Test File Organisation.

Tips

If you're not sure what are the test parameters:

1. Start with a copy of the test case
2. Change the copy
3. See where they differ
4. Refactor into a single parametrized test case

5.6.5.1 Pros 👍

• Decreased maintenance cost due to reduced duplication
• Readable test output

5.6.5.3 Do ✔️

class TestDatetimeToIsoZulu:
    @staticmethod
    @pytest.mark.parametrize(
        "instance", [
            pytest.param(datetime.now(), id="datetime without timezone"),
            pytest.param(None, id="None"),
        ]
    )
    def should_refuse(instance: Any):
        assert ...

5.6.5.4 Don't ✖️

• Don't use the same assertions in multiple tests, unless the function bodies are too different:

  class TestDatetimeToIsoZulu:
      @staticmethod
      def should_refuse_datetime_without_timezone():
          assert ...
      
      @staticmethod    
      def should_refuse_none():
          assert ...

• Don't use default example IDs:

  class TestDatetimeToIsoZulu:
      @staticmethod
      @pytest.mark.parametrize("instance", [[datetime.now()], [None]])
      def should_refuse(instance: Any):
          assert ...

• Don't parametrize values that don't change:

  class TestDatetimeToIsoZulu:
      @staticmethod
      @pytest.mark.parametrize(
          "instance,key", [
              pytest.param(datetime.now(), "key_name", id="datetime without timezone"),
              pytest.param(None, "key_name", id="None"),
          ]
      )
      def should_refuse(instance: Any, key: str):
          # `key` is identical in all examples 
          assert getattr(obj, key)

• Don't parametrize if there is just one example, unless you expect more in the next bulk of work:

  class TestDatetimeToIsoZulu:
      @staticmethod
      @pytest.mark.parametrize(
          "instance", [
              pytest.param(datetime.now(), "key_name", id="datetime without timezone"),
          ]
      )
      def should_refuse(instance: Any):
          assert ...

5.6.6 Signposting in Tests

Tests and especially integration tests can be complex. It may not be evident from reading the code what is the setup, run and the actual test. If the test is more than a couple of lines, consider adding signpost comments borrowed from the Gherkin syntax: GIVEN, WHEN, THEN

Don't use the full Gherkin syntax though. If you have a need to explain a complicated test, prefer doing so using descriptive variables and/or functions. Only if it doesn't help, use regular (block) comments.

5.6.6.1 Pros 👍

• Better structured tests because of thinking in term of given-when-then.

5.6.6.3 Do ✔️

def should_give_lower_rating_to_recipes(
    self, es_client: Elasticsearch, recipe_index: str, attribute: str, value: Any
):
    # GIVEN
    worse_recipe = _matching_recipe()
    setattr(worse_recipe, attribute, value)
    recipe_ids_in_relevance_order = _index_recipes(es_client, recipe_index, [_matching_recipe(), worse_recipe])
    
    # WHEN
    response = CLIENT.get("/recipes", params=self.REQUIRED_QUERY_PARAMS)
    
    # THEN
    assert response.status_code == HTTP_200_OK, response.text
    
    recipe_response = RecipeResponse(**response.json())
    assert [
        result.id for result in recipe_response.result
    ] == recipe_ids_in_relevance_order, "Exactly 2 IDs were expected in this order."

5.6.6.4 Don't ✖️

• Don't leave long tests without signposting:

  def should_give_lower_rating_to_recipes(
      self, es_client: Elasticsearch, recipe_index: str, attribute: str, value: Any
  ):
      worse_recipe = _matching_recipe()
      setattr(worse_recipe, attribute, value)
      
      recipe_ids_in_relevance_order = _index_recipes(es_client, recipe_index, [_matching_recipe(), worse_recipe])
      
      response = CLIENT.get("/recipes", params=self.REQUIRED_QUERY_PARAMS)
      
      assert response.status_code == HTTP_200_OK, response.text  
      recipe_response = RecipeResponse(**response.json())
      assert [
          result.id for result in recipe_response.result
      ] == recipe_ids_in_relevance_order, "Exactly 2 IDs were expected in this order."

• Don't use signposting when each section is one statement or evident:

  def should_accept_page_number(self):
      # GIVEN
      page = 1
      
      # WHEN
      response = CLIENT.get("/recipes", params={"page": page})
      
      # THEN
      assert response.status_code == HTTP_200_OK, response.text

• Don't use the full Gherkin syntax:

  def should_give_lower_rating_to_recipes(
      self, es_client: Elasticsearch, recipe_index: str, attribute: str, value: Any
  ):
      # GIVEN I have a recipe with default values
      # AND I create a recipe with one stat worse
      worse_recipe = _matching_recipe()
      setattr(worse_recipe, attribute, value)
      
      # AND I index both recipes in the relevance order I expect to get them back
      recipe_ids_in_relevance_order = _index_recipes(es_client, recipe_index, [_matching_recipe(), worse_recipe])
      
      # WHEN I make a request
      response = CLIENT.get("/recipes", params=self.REQUIRED_QUERY_PARAMS)
      
      # THEN the request is successful
      assert response.status_code == HTTP_200_OK, response.text
      
      # AND IDs order matches the expected relevance order
      recipe_response = RecipeResponse(**response.json())
      assert [
          result.id for result in recipe_response.result
      ] == recipe_ids_in_relevance_order, "Exactly 2 IDs were expected in this order."

5.7 mypy (static type checks)

5.8 invoke (tasks automation)

Prefer to use invoke framework for tasks automation. Invoke is a Python task execution tool & library, drawing inspiration from various sources to arrive at a powerful & clean feature set.

Q: Can I use Invoke tasks in pre-commit?

Yes you can. See the following .pre-commit-config.yaml file example for running a format task that executes the black formatter through pipenv:

repos:
- repo: local
  hooks:
    - id: formatting
      name: formatting
      stages: [commit]
      language: system
      pass_filenames: false
      require_serial: true
      entry: pipenv run inv format
      types: [python]

5.8.1 Pros 👍

• Deduplication of tasks (similar to Makefile)
• Removes a lot of complexity from running processes from Python - input/output handling, command line options parsing
• No new language knowledge needed
• Easy to learn

5.8.2 Cons 👎

• Harder to debug than plain Python script

5.8.3 Do ✔️

• Use the same name for tasks with the same purpose across multiple repositories. It reduces cognitive overhead when switching between repositories.

  cd apps/sparks-jobs
  invoke verify-all
  cd ../recipe-search-svc
  invoke verify-all

  Standard task names: build, format, lint, lint-docstyle, lint-pycodestyle, lint-pylint, test-all, test-integration, test-unit, typecheck, verify-all. Task names should be in kebab case.

5.8.4 Don't ✖️

• Don't rename tasks if not needed. Simply name the function as the task if possible:

  @task(name="lint-docstyle")  # can be just `@task`
  def lint_docstyle(ctx, environment):
    ...

• Don't use other task automation technologies if possible:

  • Script section of Pipfile - no benefit over calling invoke directly.
  • Ad-hoc Python scripts - there is no common interface.
  • Makefiles or Bash scripts - not everyone knows them on sufficient level to be able to fully understand them or write them well.
  • Fabric - Invoke is used already in many repositories. Using a single framework for tasks automation has lower cognitive overhead.

6 How to Extend This Guide

This is a live document and everyone is welcome to improve it. Feel free to open a PR in this repository to suggest changes or gather feedback.

6.1 Editing tips

• Prefer adding content to end of sections to prevent broken links and tedious relabeling of all subsequent sections.
• Do include section numbering (e.g. 2.4.3.1) in the section title for do and don't items. These headings generate anchors that can be used in PRs to link to the particular example directly. Without a section numbering, it won't have a unique anchor.
• There is a section template at the end of this document, hidden in a block comment. It has a lot of TODO comments to help you capture all information in a consistent style.

Footnotes

1. Only via cached_property so it won't work on methods with arguments, only properties. ↩ ↩²

2. Only with the cache outside the dataclass. So all instances have to share the same cache. ↩

3. Support via asyncache ↩ ↩² ↩³ ↩⁴