-
Notifications
You must be signed in to change notification settings - Fork 593
Contributing Code 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.
Follow these conventions when making changes to the Magma codebase.
- The project's principal convention is the boy scout rule: leave it better than you found it
- 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
- Python: directly-adjacent
- 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
- 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
- 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 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
- 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
- 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
- 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
- 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
- Good:
- 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
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.
- Magma-specific
- Use long form of CLI flags
-
--deployment
rather than-d
.
-
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.
- Good:
- Default to sentence-casing listables
- Good:
- Default to sentence-casing listables
- Bad:
- default to sentence-casing listables
- Good:
- Use hyphens for unordered lists, not asterisks
- Good:
- Magma
- Bad:
* Magma
- Good:
- Use flat apostrophes and quotes, not curly
- Good:
Magma's
,"Magma"
- Bad:
Magma’s
,“Magma”
- Good:
- 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
- Good:
- 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
- Good:
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
- 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
- Use the
golang/glog
package for all logging- Default to
-v=0
for all services
- Default to
- 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.
-
- Verbify function names
- Exception: composable method names with well-understood functionality, e.g.
foo.Filter(...).Keys().Sorted()
- Exception: using
new*
orNew*
when instantiating new objects
- Exception: composable method names with well-understood functionality, e.g.
- When import aliasing is required, prefer to alias with
snake_case
rather thancamelCase
- Prefer readable code over rigid adherence to max line lengths. Capping around 140 characters feels about right.
The PEP 8 style guide is authoritative.
- All new code should be fully type-annotated
- For reference, please look at this type hints cheat sheet for Python 3
- 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)
- Use the logging module for all logging
- Refer to the Go logging section for deciding between log levels
- 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
- On CI, the check gets run as part of the
- Additionally, we have a Reviewdog linter using wemake-python-styleguide enabled to aid the code review process
- 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.
The Google C++ Style Guide is authoritative.
- 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
- 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
- Always include what you use
- cpplint has include-what-you-use warnings
- 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
- For non-OAI C++ services, use the
MLOG
macros defined inorc8r/gateway/c/common/logging/magma_logging.h
- For OAI, use the
OAILOG_*
macros defined inlte/gateway/c/core/oai/common/log.h
- Refer to the Go logging section for deciding between log levels
Where applicable, the Google C++ Style Guide should be followed.
- 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 andstatus_or
structs should be used as the return value - Error codes and
status_or
structs can be found inlte/gateway/c/core/oai/common/common_defs.h
- A standard
- 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 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
- Reference:
- When a shell script passes around 100 lines, it's time to re-write it in Python or Go
- Import order should be: types, default components, and named components with each separated by a newline
- Use
TitleCase
for component file names - Favor
map
andforEach
functions in place of regularfor
loops - Refrain from using literal strings/numbers without defining them
- Use strict equality(
===
) when comparing values
- 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
- On CI, the check gets run as part of the
- 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
- 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
- On CI, the check gets run as part of the
- 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
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 thereserved
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
- E.g.
- 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 andsnake_case
for field names - Two-space indents
- Routes always return an object (forward compatibility)
- 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
- Consolidate related functionality into a single CLI
- 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 onB
andC
. Also letB
have a dependency onC
then the dependency should be defined atB
. -
srcs
anddeps
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.
- A service binary should only have the respective
main.py
as source and other needed source files wrapped in apy_library
dependency. E.g. a targetmy_service
(py_binary
) has a dependency onmy_service_lib
(py_library
). - Do not edit
requirements.txt
directly but generate it fromrequirements.in
.
Magma Website • Docs • Code • Contributing • Wiki