github.com/m3db/m3@v1.5.1-0.20231129193456-75a402aa583b/STYLEGUIDE.md (about)

     1  # M3 Coding Styleguide
     2  
     3  M3's umbrella coding style guide is Uber's [Go Style Guide][uber-guide]. This
     4  document is maintained as a superset of that guide, capturing any substantive,
     5  intended departures from Uber's coding style. Where possible, code should follow
     6  these guidelines to ensure consistent style and coding practices across the
     7  codebase.
     8  
     9  Above all else, this guide is intended to be a point of reference rather than
    10  a sacred text. We maintain a style guide as a pragmatic tool that can be used to
    11  avoid common issues and normalize code across many authors and the Go community.
    12  
    13  New code should follow the style guide by default, preferring guidelines
    14  established here or in Uber's guide over any conflicting, pre-existing
    15  precedents in the M3 codebase. Ultimately, the hope is that the codebase
    16  incrementally moves closer to the style guide with each change.
    17  
    18  Since the M3 monorepo predated this style guide, reviewers should not expect
    19  contributors to make unrelated or unreasonable style-based changes as part of
    20  pull requests. However, when changing code that could reasonably be updated
    21  to follow the guide, we prefer that those changes adopt the guidelines to avoid
    22  sustaining or increasing technical debt. See DEVELOPMENT.md for more detail on
    23  changes involving style.
    24  
    25  [uber-guide]: https://github.com/uber-go/guide/blob/master/style.md
    26  
    27  ## Linting
    28  
    29  Many guidelines are flagged by `go vet` or the other configured linters (see
    30  [.golangci.yml][.golangci.yml]). Wherever possible, we prefer to use tooling to
    31  enforce style to remove subjectivity or ambiguity. Linting is also a blocking
    32  build for merging pull requests.
    33  
    34  [.golangci.yml]: https://github.com/m3db/m3/blob/master/.golangci.yml
    35  
    36  ## Template
    37  
    38  When adding to this guide, use the following template:
    39  
    40  ~~~
    41  ### Short sentence about the guideline.
    42  
    43  Clearly (and succinctly) articulate the guideline and its rationale, including
    44  any problematic counter-examples. Be intentional when using language like
    45  "always", "never", etc, instead using words like "prefer" and "avoid" if the
    46  guideline isn't a hard rule. If it makes sense, also include example code:
    47  
    48  <table>
    49  <thead><tr><th>Bad</th><th>Good</th></tr></thead>
    50  <tbody>
    51  <tr><td>
    52  
    53  ```go
    54  goodExample := false
    55  ```
    56  
    57  </td><td>
    58  
    59  ```go
    60  goodExample := true
    61  ```
    62  
    63  </td></tr>
    64  <tr><td>
    65  Description of bad code.
    66  </td><td>
    67  Description of good code.
    68  </td></tr>
    69  </tbody></table>
    70  ~~~
    71  
    72  ## Guidelines
    73  
    74  ### Export types carefully.
    75  
    76  Types should only be exported if they must be used by multiple packages. This
    77  applies also to adding new packages: a new package should only be added if it
    78  will be imported by multiple packages. If a given type or package will only
    79  initially be imported in one package, define those type(s) in that importing
    80  package instead.
    81  
    82  In general, it's harder to reduce surface area than it is to incrementally
    83  increase surface area, and the former is a breaking change while the latter is
    84  often not.
    85  
    86  ### Treat flaky tests like consistent failures.
    87  
    88  Flaky tests add noise to code health signals, reduce trust in tests to be
    89  representative of code behavior. Worse, flaky tests can be either false positive
    90  or false negative, making it especially unclear as to whether or not a given
    91  test passing or failing is good or bad. All of these reduce overall velocity
    92  and/or reliability.
    93  
    94  All tests discovered to be flaky should be immediately result in either (a) the
    95  test being skipped because it is unreliable, or (b) master being frozen until
    96  the test is fixed and proven to no longer be flaky.
    97  
    98  ### Do not expose experimental package types in non-experimental packages.
    99  
   100  A package is only able to guarantee a level of maturity/stability that is the
   101  lowest common denominator of all of its composing or transitively exported
   102  types. Given a hypothetical scenario:
   103  
   104  ```go
   105  package foo
   106  
   107  type Bar {
   108    Baz xfoo.Baz
   109  }
   110  ```
   111  
   112  In this case, the stability of `foo.Bar` is purportedly guaranteed by package
   113  `foo` being non-experimental, but since it transitively exposes `xfoo.Baz` as
   114  part of `foo.Bar`, either (a) `xfoo.Baz` must implicitly adhere to versioning
   115  compatibility guarantees or (b) `foo` can no longer be considered stable,
   116  as any breaking change to `xfoo.Baz` will break `foo`.
   117  
   118  This is spiritually similar to the
   119  [Avoid Embedding Types In Public Structs][avoid-embedding-types] guidance, in
   120  that it bleeds implementation and compatibility details in an inappropriate way.
   121  
   122  This guidance also applies to any cases in which `internal` packages are used:
   123  any `internal` type is essentially the same as an unexported type, meaning that
   124  that type is only implicitly available to users.
   125  
   126  [avoid-embedding-types]: https://github.com/uber-go/guide/blob/master/style.md#avoid-embedding-types-in-public-structs
   127  
   128  <table>
   129  <thead><tr><th>Bad</th><th>Good</th></tr></thead>
   130  <tbody>
   131  <tr><td>
   132  
   133  ```go
   134  type NewConnectionFn func(
   135      channelName string, addr string, opts Options,
   136  ) (xclose.SimpleCloser, rpc.TChanNode, error)
   137  ```
   138  
   139  </td><td>
   140  
   141  ```go
   142  type NewConnectionFn func(
   143      channelName string, addr string, opts Options,
   144  ) (io.Closer, rpc.TChanNode, error)
   145  
   146  // or
   147  
   148  type SimpleCloser = func()
   149  
   150  type NewConnectionFn func(
   151      channelName string, addr string, opts Options,
   152  ) (SimpleCloser, rpc.TChanNode, error)
   153  ```
   154  
   155  </td></tr>
   156  <tr><td>
   157  
   158  `xclose.SimpleCloser` is part of `x/close`, an experimental package, but is
   159  directly exposed as part of `src/dbnode/client.NewConnectionFn`.
   160  
   161  </td><td>
   162  
   163  The canonical `io.Closer` is used instead, or a type alias representing
   164  `xclose.SimpleCloser` is used instead. Both options prevent leaking experimental
   165  packages as part of non-experimental library APIs.
   166  
   167  </td></tr>
   168  </tbody></table>