Changes between Version 22 and Version 23 of CodingStandards


Ignore:
Timestamp:
2013-05-23T17:59:30Z (12 years ago)
Author:
daira
Comment:

change policies on implicit coercion to boolean and -O

Legend:

Unmodified
Added
Removed
Modified

  • CodingStandards

    v22 v23  
    4444
    4545 * Don't use the literals {{{True}}} or {{{False}}} in conditional expressions -- instead just write the expression which will evaluate to true or false. For example, write {{{if expr:}}} instead of {{{if expr == True:}}} and {{{if not expr:}}} instead of {{{if expr == False:}}}.
    46  * ''Disputed'': Use the fact that empty sequences, empty strings, empty dicts, {{{0}}}, and {{{None}}} all evaluate to false. Write {{{if not items:}}} instead of {{{if len(items) == 0:}}}.
    47    * I disagree with relying on implicit conversion to boolean; I think it's error-prone (and we have had real bugs because of it). -- David-Sarah
    48  * But if your intent is to test for {{{None}}} instead of to test for "any false thing", then write it out as {{{if thing is None:}}}.
     46 * Avoid relying on the fact that empty sequences, empty strings, empty dicts, {{{0}}}, and {{{None}}} are treated as false. Write {{{if len(items) == 0:}}}, {{{if thing is None:}}}, etc.
    4947
    5048== advanced idioms ==
    … …  
    7876The "error message" that will accompany a failed expression should be a statement of what is required for correct operation. Don't write something like "Spam isn't firm.", because that is ambiguous: the error could be that the spam is supposed to be firm and it isn't, or the error could be that spam isn't supposed to be firm and it is! The same ambiguity can apply to the sentence "Spam must be firm.".  It helps to use the words "required to" in your message, for example "Spam is required to be firm.".
    7977
     78Assertions are not a substitute for proper error handling! An assertion, precondition or postcondition should only be used for cases that "cannot happen" unless the code is incorrect. They should not be used to check for invalid inputs; in that case, raise an exception instead.
     79
     80==== avoid "bare assert" ====
     81
     82Python's built-in {{{assert}}} statement, unlike {{{allmydata.util.assertutil._assert}}}, can be switched off by the {{{-O}}} option to {{{python}}} or the {{{PYTHONOPTIMIZE}}} environment variable. Although this might sound useful to reduce the overhead of assertions, in practice that overhead is negligable, and conditional assertions are more trouble than they're worth (partly because they create a configuration that is mostly untested). We are in the process of removing all bare asserts from the codebase (#1968).
     83
    8084==== class invariants ====
    8185
    … …  
    98102Now you can put {{{assert self._assert_invariants()}}} everywhere in your class where the class ought to be in an internally consistent state. For example, at the beginning of every externally-callable method. This technique can be very valuable in developing a complex class -- it catches bugs early, it isolates bugs into specific code paths, and it clarifies the internal structure of the class so that other developers can hack on it without subtle misunderstandings.
    99103
    100 * we actually appear to only have one instance of this pattern in Tahoe at time of writing, in {{{allmydata.util.dictutil}}}. It has the disadvantage of cluttering up the logic with calls to {{{_assert_invariants}}}, and should probably be used sparingly. -- David-Sarah
     104* we actually appear to only have one instance of this pattern in Tahoe at time of writing, in {{{allmydata.util.dictutil}}}. It has the disadvantage of cluttering up the logic with calls to {{{_assert_invariants}}}, and should probably be used sparingly. -- Daira
    101105
    102106==== assertion policy ====
    … …  
    118122callers.
    119123
    120  * The general rule is that nodes must be functional for light traffic even
    121    when the assertions are turned on. When assertions are turned off (-O),
    122    nodes must be functional for heavy traffic.
     124 * The general rule is that assertions must not prevent nodes from being functional,
     125   even for heavy traffic.
    123126
    124  * Time-consuming internal checks: once the code is working properly,
    125    consider removing them, but they may be left in place as long as they
    126    use {{{assert}}} (the form which gets turned off when -O is used).
     127 * Time-consuming internal checks: once the code is working properly, consider
     128   removing them, or making them conditional on a module-level DEBUG constant.
    127129
    128  * Cheap internal checks: once the code is working properly, consider
    129    removing them, but it is less of a concern than the time-consuming ones.
    130    If they really are cheap, use {{{_assert}}} (the unconditional form
    131    that gets used even with -O).
     130 * Cheap internal checks: keep them even after the code is working properly.
    132131
    133  * Time-consuming external checks: maybe leave them in place, but always
    134    use {{{assert}}} so they will not be used with -O.
    135 
    136  * Cheap external checks: leave them in place, using the unconditional
    137    {{{_assert}}}
    138 
    139  * Production grids could run with -O (in practice, the allmydata.com production grid runs without -O, because there are no expensive checks in the current codebase).
    140 
    141  * Testing grids might run without -O in order to detect more bugs.
    142 
    143  * Local developer tests will probably not use -O, and developers should be
    144    prepared to experience the same CPU load problems if they subject their
    145    nodes to real traffic levels. Developers can use -O to turn off everyone
    146    else's checks, use {{{_assert}}} on their own code to enable their own
    147    assertions, and then subject their nodes to heavy traffic, as long as they
    148    are sure to change their checks to use {{{assert}}} (or remove them
    149    altogether) before committing.
     132 * External checks: keep them if they are necessary for security or robustness
     133   (but probably by raising an exception rather than as an assertion). If they
     134   are not necessary for security or robustness, treat in the same way as internal
     135   checks.
    150136
    151137