|
| 1 | +Coding Standards |
| 2 | +================ |
| 3 | + |
| 4 | +The coding standards for volatility are mostly by our linter and our code formatter. |
| 5 | +All code submissions will be vetted automatically through tests from both and the submission will not be accepted if either of these fail. |
| 6 | + |
| 7 | +Code Linter: Ruff |
| 8 | +Code Formatter: Black |
| 9 | + |
| 10 | +In addition, there are some coding practices that we employ to prevent specific failure cases and ensure consistency across the codebase. These are documented below along with the rationale for the decision. |
| 11 | + |
| 12 | +This is heavily based upon https://google.github.io/styleguide/pyguide.html with minor modifications for volatility use. |
| 13 | + |
| 14 | +Imports |
| 15 | +------- |
| 16 | + |
| 17 | +Use import statements for packages and modules only, not for individual types, classes, or functions and ideally not aliased unless the imported name would cause confusion. This is to prevent people from importing something that was itself imported from elsewhere (which can lead to confusion and add in an unnecessary dependency in the import chain). |
| 18 | + |
| 19 | +* Use `import x` for importing packages and modules. |
| 20 | +* Use `from x import y` where x is the package prefix and y is the module name with no prefix. |
| 21 | +* Use `from x import y as z` in any of the following circumstances: |
| 22 | + * Two modules named `y` are to be imported. |
| 23 | + * `y` conflicts with a top-level name defined in the current module. |
| 24 | + * `y` conflicts with a common parameter name that is part of the public API (e.g., `features`). |
| 25 | + * `y` is an inconveniently long name. |
| 26 | + * `y` is too generic in the context of your code (e.g., `from storage.file_system import options as fs_options`). |
| 27 | + |
| 28 | +Exemptions from this rule: |
| 29 | + |
| 30 | + * Symbols from the following modules are used to support static analysis and type checking: |
| 31 | + * `typing` module |
| 32 | + * `collections.abc` module |
| 33 | + * `typing_extensions` module |
| 34 | + |
| 35 | +Function calls |
| 36 | +-------------- |
| 37 | + |
| 38 | +For longer function calls, where line length is no longer an issue, favour using keyword arguments for clarity over unnamed positional arguments. |
| 39 | +This helps coders learning the code from examples to know what parameters to pass in and avoids ordering mistakes. |
| 40 | + |
| 41 | +Global Mutable State |
| 42 | +-------------------- |
| 43 | + |
| 44 | +Avoid mutable global state. |
| 45 | + |
| 46 | +In those rare cases where using global state is warranted, mutable global entities should be declared at the module level or as a class attribute and made internal by prepending an _ to the name. If necessary, external access to mutable global state must be done through public functions or class methods. See Naming below. Please explain the design reasons why mutable global state is being used in a comment or a doc linked to from a comment. |
| 47 | + |
| 48 | +Module-level constants are permitted and encouraged. For example: _MAX_HOLY_HANDGRENADE_COUNT = 3 for an internal use constant or SIR_LANCELOTS_FAVORITE_COLOR = "blue" for a public API constant. Constants must be named using all caps with underscores. See Naming below. |
| 49 | + |
| 50 | +Exceptions |
| 51 | +---------- |
| 52 | + |
| 53 | +Never use catch-all except: statements, or catch Exception or StandardError, unless you are |
| 54 | + |
| 55 | + * re-raising the exception, or |
| 56 | + * creating an isolation point in the program where exceptions are not propagated but are recorded and suppressed instead, such as protecting a thread from crashing by guarding its outermost block. |
| 57 | + |
| 58 | +Python is very tolerant in this regard and except: will really catch everything including misspelled names, sys.exit() calls, Ctrl+C interrupts, unittest failures and all kinds of other exceptions that you simply don’t want to catch. |
| 59 | + |
| 60 | +Versioning |
| 61 | +---------- |
| 62 | + |
| 63 | +Modules that inherit from `VersionableInterface` define a `_version` attribute which states their version. This is a tuple of `(MAJOR, MINOR, PATCH)` numbers, which can then be used for Semantic Versioning (where modifications that change the API in a non-backwards compatible way bump the `MAJOR` version (and set the `MINOR` and `PATCH` to 0) and additive changes increase the `MINOR` version (and set the `PATCH` to 0). Changes that have no effect on the external interface (either input or output form) should have their `PATCH` number incremented. This allows for callers of the interface to determine when changes have happened and whether their code will still work with it. Volatility carries out these checks through the requirements system, where a plugin can define what requirements it has. |
| 64 | + |
| 65 | +Shared functionality |
| 66 | +-------------------- |
| 67 | + |
| 68 | +Within a plugin, there may be functions that are useful to other plugins. These are created as `classmethod`s so that the plugin can be depended upon by other plugins in their requirements section, without needing to instantiate a whole copy of the plugin. It is not a staticmethod, because the caller may wish to determine information about the class the method is defined in, and this is not easily accessible for staticmethods. |
| 69 | +A classmethod usually takes a `context` for its first method (and if it requires one, a configuration string for it second). All other parameters should generally be basic types (such as strings, numbers, etc) so that future work requiring parallelization does not have complex types to have to keep in sync. In particular, the idea was to ensure only one context was used per method (and each object brings its own context with it, meaning the function signature should not include objects to avoid discrepancies). |
| 70 | + |
| 71 | +Comprehensions |
| 72 | +-------------- |
| 73 | + |
| 74 | +Comprehensions are allowed, however multiple for clauses or filter expressions are not permitted. Optimize for readability, not conciseness. |
| 75 | + |
| 76 | +Lambda functions |
| 77 | +---------------- |
| 78 | + |
| 79 | +Okay for one-liners. Prefer generator expressions over map() or filter() with a lambda. |
| 80 | + |
| 81 | +Default Arguments |
| 82 | +----------------- |
| 83 | + |
| 84 | +Default arguments are fine, but not with mutable types (because they're constructed once at module load time and can lead to confusion/errors.) |
| 85 | + |
| 86 | +Format strings |
| 87 | +-------------- |
| 88 | +Generally f-strings are preferred, and where possible a format modifier should be used over a separate method call. As an example, hex output should be `f"0x{offset:x}"` rather than `f"{hex(offset)}"`. |
| 89 | +F-strings should be used over other formatting methods *except* in cases of logging where the f-string gets calculated/executed whether the log message is displayed or not (where as parameters are not evaluated if not needed). |
| 90 | +The ruff linter should alert about these situations and exceptions can be maded if needed. |
| 91 | + |
| 92 | +True/False Evaluations |
| 93 | +---------------------- |
| 94 | + |
| 95 | +Use the “implicit” false if possible, e.g., if foo: rather than if foo != []:. There are a few caveats that you should keep in mind though: |
| 96 | + |
| 97 | + * Always use `if foo is None:` (or `is not None`) to check for a `None` value. E.g., when testing whether a variable or argument that defaults to `None` was set to some other value. The other value might be a value that’s false in a boolean context! |
| 98 | + * Never compare a boolean variable to `False` using `==`. Use `if not x:` instead. If you need to distinguish `False` from `None` then chain the expressions, such as `if not x and x is not None:`. |
| 99 | + * For sequences (strings, lists, tuples), use the fact that empty sequences are false, so `if seq:` and `if not seq:` are preferable to `if len(seq):` and `if not len(seq):` respectively. |
| 100 | + |
| 101 | +Logging |
| 102 | +------- |
| 103 | + |
| 104 | +We do allow f-string usage in log messages, although technically it should be avoided since it will be evaluated even if the log message is never emitted. |
0 commit comments