From 6f5f795e47fa1033c793ba521a852f8323fa3bd0 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Mon, 8 Oct 2018 17:46:11 +0900 Subject: CONTRIBUTING.rst: Added section about minimizing API surfaces And modified some titles so that the titles in CONTRIBUTING.rst actually follow the documentation guidelines for naming of section titles. --- CONTRIBUTING.rst | 60 ++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 8 deletions(-) (limited to 'CONTRIBUTING.rst') diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 4d368ebe4..20647197c 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -496,7 +496,7 @@ outline the exceptions to the rules discussed here. .. _contributing_public_api_surface: -Public API Surface +Public API surface ~~~~~~~~~~~~~~~~~~ BuildStream exposes what we call a *"Public API Surface"* which is stable and unchanging. This is for the sake of stability of the interfaces which @@ -576,7 +576,9 @@ This makes things clear when reading code that said functions are not defined in the same file but come from utils.py for example. -Instance Variables +.. _contributing_instance_variables: + +Instance variables ~~~~~~~~~~~~~~~~~~ It is preferred that all instance state variables be declared as :ref:`private symbols `, however in some cases, especially when the state @@ -607,7 +609,7 @@ variable. .. _contributing_accessor_mutator: -Accessors and Mutators +Accessors and mutators ~~~~~~~~~~~~~~~~~~~~~~ An accessor and mutator, are methods defined on the object class to access (get) or mutate (set) a value owned by the declaring class, respectively. @@ -667,7 +669,7 @@ respectively, e.g.:: .. _contributing_abstract_methods: -Abstract Methods +Abstract methods ~~~~~~~~~~~~~~~~ In BuildStream, an *"Abstract Method"* is a bit of a misnomer and does not match up to how Python defines abstract methods, we need to seek out @@ -695,7 +697,7 @@ to do so, and outlined the API contract for this purpose. No surprises are allowed. -Error Handling +Error handling ~~~~~~~~~~~~~~ In BuildStream, all non recoverable errors are expressed via subclasses of the ``BstError`` exception. @@ -704,7 +706,7 @@ This exception is handled deep in the core in a few places, and it is rarely necessary to handle a ``BstError``. -Raising Exceptions +Raising exceptions '''''''''''''''''' When writing code in the BuildStream core, ensure that all system calls and third party library calls are wrapped in a ``try:`` block, @@ -721,7 +723,7 @@ original call trace, example:: raise ArtifactError("Attempt to access unavailable artifact: {}".format(e)) from e -Enhancing Exceptions +Enhancing exceptions '''''''''''''''''''' Sometimes the ``BstError`` originates from a lower level component, and the code segment which raised the exception did not have enough context @@ -913,7 +915,7 @@ if the developer does not take special care of ensuring this does not happen. -Use less arguments in methods +Minimize arguments in methods ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ When creating an object, or adding a new API method to an existing object, always strive to keep as much context as possible on the @@ -926,6 +928,48 @@ that value or object should be passed in the constructor instead of via a method call. +Minimize API surfaces +~~~~~~~~~~~~~~~~~~~~~ +When creating an object, or adding new functionality in any way, +try to keep the number of :ref:`public, outward facing ` +symbols to a minimum, this is important for both +:ref:`internal and public, plugin facing API surfaces `. + +When anyone visits a file, there are two levels of comprehension: + +* What do I need to know in order to *use* this object + +* What do I need to know in order to *modify* this object + +For the former, we want the reader to understand with as little effort +as possible, what the public API contract is for a given object and consequently, +how it is expected to be used. This is also why we +:ref:`order the symbols of a class ` in such a way +as to keep all outward facing public API surfaces at the top of the file, so that the +reader never needs to dig deep into the bottom of the file to find something they +might need to use. + +For the latter, when it comes to having to modify the file or add functionality, +you want to retain as much freedom as possible to modify internals, while +being sure that nothing external will be affected by internal modifications. +Less client facing API means that you have less surrounding code to modify +when your API changes. Further, ensuring that there is minimal outward facing +API for any module minimizes the complexity for the developer working on +that module, by limiting the considerations needed regarding external side +effects of their modifications to the module. + +When modifying a file, one should not have to understand or think too +much about external side effects, when the API surface of the file is +well documented and minimal. + +When adding new API to a given object for a new purpose, consider whether +the new API is in any way redundant with other API (should this value now +go into the constructor, since we use it more than once ? could this +value be passed along with another function, and the other function renamed, +to better suit the new purposes of this module/object ?) and repurpose +the outward facing API of an object as a whole every time. + + Avoid transient state on instances ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ At times, it can be tempting to store transient state that is -- cgit v1.2.1