Contributing Conventions

This document describes contributing conventions for the Magma project. The goal of this style guide is to

• steer contributors away from experienced landmines and
• align on coding styles in support of a uniform Magma codebase, with the aim to improve developer productivity and codebase approachability.

General

Follow these conventions when making changes to the Magma codebase.

Leave it better than you found it

• The project's principal convention is the boy scout rule: leave it better than you found it

Add tests for your changes

• Tests should cover, at minimum, the mainline of the new feature. For reference, that usually ends up around 50-70% coverage.
• Unit tests should default to being placed in the same directory as the files they're testing, except for the following
  • Python: directly-adjacent tests directory
  • C/C++: directly-adjacent tests directory
  • JavaScript: directly-adjacent __tests__ directory
• Integration tests should be placed as close to the code-under-test as possible
• If you're not sure how to test a change, reach out on the community Slack workspace for input

Separate cleanup PRs from functional changes

• Keeps PRs small and understandable
• Exception: if the area of the codebase you're editing needs a cleanup PR, but you don't have bandwidth to add one, default to mimicking the surrounding code

Scope component responsibilities

• Functions, and components in general, should be narrowly scoped to a single, specific role
• When writing a function over 100 lines long, consider extracting a helper functions out of the intermediate logical steps

Prefer immutability and idempotency

• Prefer immutable state
• When mutability is necessary, consider the following
  • Prefer to set a component's state entirely in its constructor
  • Mutate a component's state as close to construction-time as possible
  • Perform mutations as high in the call chain as possible
• Prefer side-effect-free functions
  • When side-effects are necessary, move them as high in the call chain as possible

Prefer composition over inheritance

• Avoid inheritance as a design pattern, in favor of composition and dependency injection
• If complex logic begins bleeding into test case setup, consider pulling that logic into a dependency interface
• Build a complex component as a composition of multiple simpler components with clear interfaces
• Avoid non-trivial static functions: pull interfaces out of the static functions and inject them into depending components

Use simple constructors

• Split complex logic and side-effect-inducing functionality out of the constructor and into an initialization method
• If desired, can also use a static factory function to construct the component and call its initialization method

Comment with why, not what

• Good code is self-documenting
• Instead of defaulting to inline comments, focus on
  • Concise and descriptive identifier names
  • Intelligent types and pervasive typing information
  • High-quality docstrings on functions, components, and top-level identifiers
• Avoid "topic sentence" comments
  • E.g. "this block of code does X ... this block of code does Y", when there's no value-add other than summarizing the next few lines
  • Instead, code paragraphs should be skimmable
  • Consider breaking dense code paragraphs out into private functions.
• Save comments for code blocks that require non-obvious context, e.g. answering why an idiosyncratic or non-obvious decision was made

Follow style conventions

• Use Go-style doc comments, where the doc comment is prefixed by the name of the object being documented
• Use Americanized spellings
  • marshaling, marshaled
  • canceling, canceled, cancellation
• Use alphabetized metasyntactic variables
  • Good: apple, banana, cherry, date, egg
  • Bad: foo, bar, baz, quz, soap
• Prefer underscores over hyphens
  • File, directory names
  • YAML, JSON
  • Exception: in certain parts of K8s, underscores are disallowed. In this case, hyphens are preferred, and translation between hyphens and underscores is acceptable.
• Omit trailing slash of directory paths, except where semantically meaningful
• Don't terminate new service names with d

Documentation

All documentation

The end-goal is uniform, approachable documentation, especially in the Docusaurus

• Write in plain English
  • Short sentences
  • Active verbs
  • Use "you" and "we"
  • Avoid nominalisations
  • Use lists
• Use descriptive hyperlink text
  • Don't use "here" as the text for a hyperlink
• Consistent capitalization and spelling
  • Magma-specific
    • Orc8r, Orchestrator
    • NMS
    • Magma
    • Mconfig
  • Magma-adjacent
    • K8s, Kubernetes
    • Helm
    • gRPC
    • eNodeB
  • Everyday words
    • metadata
    • use-case
  • Magma service names
    • state, subscriberdb, lte, etc.
• Use long form of CLI flags
  • --deployment rather than -d.

Markdown

Consider the following Markdown conventions

• Don't wrap long lines
• Use # for headers, rather than underlining
• Place an empty line before and after a list block
• Don't preface lists with punctuation
  • Good: Consider the following Markdown conventions
  • Bad: Consider the following Markdown conventions:
  • Bad: Consider the following Markdown conventions,
  • Bad: Consider the following Markdown conventions.
• Default to sentence-casing listables
  • Good: - Default to sentence-casing listables
  • Bad: - default to sentence-casing listables
• Use hyphens for unordered lists, not asterisks
  • Good: - Magma
  • Bad: * Magma
• Use flat apostrophes and quotes, not curly
  • Good: Magma's, "Magma"
  • Bad: Magma’s, “Magma”
• Title-case H1 headers, sentence-case H2 and lower headers
  • Good: # Code Conventions in Magma, ## Code conventions in Magma
  • Bad: # Code conventions in Magma, ## Code Conventions in Magma
• Use space-padded double-hyphen to approximate an em dash
  • Good: Magma -- an open source project -- has code conventions
  • Bad: Magma--an open source project--has code conventions
  • Bad: Magma - an open source project - has code conventions

Languages

Go

Orc8r's cloud code has some basic CI lint checks. The Go style guide and anything concrete from Effective Go are authoritative. Aside from those, consider the following conventions

General

• Familiarize yourself with these 3 common Go landmines
• Check in generated code
• Avoid init functions in Magma code
  • Exception: generated code and imported code
  • If a new init function absolutely must be added, it must be idempotent, contained to its package, and not cause global state mutation
• Avoid global state
  • This includes global registries
  • Preferred alternative: pass instance of object around directly
  • Acceptable alternative: singleton instances of a private object accessed by public functions
• Default to using separate _test package for tests
  • I.e. same directory, different package
  • Example: orc8r/cloud/go/services/state/indexer/indexer_test.go
  • In almost all cases, the code-under-test should be re-writable into something that can be tested from an external test package
  • Only use same-package tests when absolutely necessary, and in that case put them in a separate test file
• When returning an error, all other returns should contain their zero value

Logging

• Use the golang/glog package for all logging
  • Default to -v=0 for all services
• Deciding when to log errors
  • There are two types of errors: platform errors, where there's something wrong with Orc8r, and client errors, where a client made an invalid request. The former must be logged to communicate the error. The latter can be logged as a high-verbosity info log, as a helpful debugging tool -- that is, client errors should not be logged as Orc8r errors
  • Prefer returning an error rather than error logging -- error logging should occur as high in the call stack as possible
  • Add an explanatory comment when swallowing errors (i.e. when choosing to log an error and not bubble it up the call stack)
• Log level
  • Fatal conservatively, and only when the service has degraded to the point of inability to function. Fatal-ing on service startup is a useful pattern, but fatal-ing after service initialization should be avoided unless absolutely necessary.
  • Error when something is definitely wrong, e.g. a violated invariant.
  • Warning when something is probably wrong, but it's not possible to be sure it's an error. This is an infrequent use-case, prefer error.
  • Info for everything else, with appropriate verbosity.

Style

• Verbify function names
  • Exception: composable method names with well-understood functionality, e.g. foo.Filter(...).Keys().Sorted()
  • Exception: using new* or New* when instantiating new objects
• When import aliasing is required, prefer to alias with snake_case rather than camelCase
• Prefer readable code over rigid adherence to max line lengths. Capping around 140 characters feels about right.

Python

The PEP 8 style guide is authoritative.

Type annotations

• All new code should be fully type-annotated
  • For reference, please look at this type hints cheat sheet for Python 3

Documentation

• Document all public functions and keep those docs up to date when you make changes
• We use Google style docstrings in our codebase
  • For VSCode users, Python Docstring Generator plugin is recommended
  • For IntelliJ users, you can configure a doc string format via Preferences->Tools->Python Integrated Tools->Docstring format

Example:

def foo(arg1: str) -> int:
    """Returns the length of arg1.

    Args:
        arg1 (str): string to calculate the length of

    Returns: the length of the provided parameter
    """
    return len(arg1)

Logging

• Use the logging module for all logging
• Refer to the Go logging section for deciding between log levels

Linter

• For mandatory lint checks, we have a unit test that runs Pylint on all gateway services
  • On CI, the check gets run as part of the lte-test job
• Additionally, we have a Reviewdog linter using wemake-python-styleguide enabled to aid the code review process
  • Instructions on running the linter locally

Formatters

• Always format Python changes locally
• We do not recommend other formatters such as black, as it diverges from pep8 on basic things like line length, etc.

C++

The Google C++ Style Guide is authoritative.

Documentation

• Always document your functions and classes in the header files over source files
• Use Doxygen style documentation for functions
  • For VSCode users, the doxygen documentation generator plugin is recommended

Types

• Be mindful when choosing input / output types when writing new functions
  • Opt for return values over output parameters
  • Non-optional input parameters should be values or const references
  • Use non-const pointers to represent optional outputs and optional input/output parameters

Headers

• Always include what you use
  • cpplint has include-what-you-use warnings

Linter

• We recommend Google's cpplint to lint your changes locally
  • For VSCode users, the cpplint plugin is recommended
• We also have a Reviewdog annotatoer that runs cpplint to aid the code review process

Logging

• For non-OAI C++ services, use the MLOG macros defined in orc8r/gateway/c/common/logging/magma_logging.h
• For OAI, use the OAILOG_* macros defined in lte/gateway/c/core/oai/common/log.h
• Refer to the Go logging section for deciding between log levels

C

Where applicable, the Google C++ Style Guide should be followed.

Exceptions, errors, and assertions

• As an exception mechanism is not provided in C, any error handling should be done either with assertions or error codes
• As a principle, library functions should never cause crashes, and higher level callers should be responsible for handling errors
• When exception handling is required, prefer returning error codes over using assertions
  • A standard status_code_e enum and status_or structs should be used as the return value
  • Error codes and status_or structs can be found in lte/gateway/c/core/oai/common/common_defs.h
• Assertions should be reserved for unrecoverable errors
  • Eg. a valid use is in MME service when out of memory, and there is no way to recover
  • Eg. an invalid use is a misformatted input message received by MME. MME should respond with an error and not crash.
  • Eg. an invalid use is for TODOs in unimplemented code. MME should respond with an error instead of crashing.

Shell

• Shell script names should be suffixed with the proper file extension
  • Reference: sh vs. bash
  • .sh for POSIX-compliant shell
  • .bash for bash
  • Default to .bash except with specific reason
• When a shell script passes around 100 lines, it's time to re-write it in Python or Go

Javascript

General

• Import order should be: types, default components, and named components with each separated by a newline
• Use TitleCase for component file names
• Favor map and forEach functions in place of regular for loops
• Refrain from using literal strings/numbers without defining them
• Use strict equality(===) when comparing values

Type annotations

• All new code should be fully type annotated
• We have a mandatory unit test that runs the Flow type checker on the NMS codebase
  • On CI, the check gets run as part of the nms-flow-test job

Documentation

• Document all the components and functions you create
• Keep the docs up to date when you make changes
• Use JSDoc tags to document your code
• Use // to comment line of code, if it is not clear in what it is doing

Linter

• Run ESLint to lint your changes locally
• For mandatory lint checks, we have a unit test that runs eslint on JS code
  • On CI, the check gets run as part of the eslint job

Dockerfile

• Dockerfile best practises should be followed
• In multiline statements, the bash logical operator is prepended. E.g.

  RUN mkdir -p /usr/src/things \
      && curl -SL https://example.com/big.tar.xz \
      |  tar -xJC /usr/src/things \
      || echo "ERROR!" \
      ;  make -C /usr/src/things all
  

Tools

gRPC

The Google Protocol Buffer style guide is authoritative. We also follow a subset of the Uber Protocol Buffer style guide. Consider the selection below

• Streaming RPCs are strongly discouraged
• When deprecating a field, use the deprecated option instead of the reserved keyword
• RPC request and return definitions should be unique to the RPC
  • E.g. rpc GetTrip(GetTripRequest) returns (GetTripResponse);
  • This is especially relevant for servicer definitions at the Orc8r-gateway interface
• Uniform file structure (example)
  • License
  • File overview
  • Syntax
  • Package
  • Imports (sorted)
  • File options
  • Everything else
    • Define services first, then their constituent objects
• Use PascalCase for message names and snake_case for field names
• Two-space indents

Swagger

• Routes always return an object (forward compatibility)

YAML

• Use the casing convention that is idiomatic for the code that will be reading the YAML file
  • Rationale: facilitates automatically unmarshaling the file to native object
  • Example: for Go config files, use camelCase
  • When a YAML file may be read by multiple languages, default to snake_case

CLIs

• Consolidate related functionality into a single CLI

Bazel (experimental)

• Use the buildifier (bazel run //:buildifier) to apply auto-format to Bazel changes.
• Define dependencies on the lowest level, e.g. let target A have a dependency on B and C. Also let B have a dependency on C then the dependency should be defined at B.
• srcs and deps arguments should always be in-lined (i.e., not given as variables) so that the buildifier can sort entries correctly.
• Use visibility = ["//visibility:private"] (default) to make more clear that a target is only used internally.

Python specifics

• A service binary should only have the respective main.py as source and other needed source files wrapped in a py_library dependency. E.g. a target my_service (py_binary) has a dependency on my_service_lib (py_library).
• Do not edit requirements.txt directly but generate it from requirements.in.