Sasview Coding Rules

The coding rules are based on few already existing rules of !SasView and other programs available from SINE2020 project (primarily Mantid http://www.mantidproject.org/Coding_Standards). The idea is to implement the coding rules at ESS and then potentially suggest the guideline for future developers within !SasView community.

The following document is divided into subsections (Some of these needs to be written)

* [#point1  Golden Rule ]   
* [#point2 Coding Standards]
        * [#point3 Docstring]
        * [#point4 Naming]
        * [#point5 Imports]
        * [#point6 Exception Handling]
        * [#point7 Coding Style]

* [#point8 Testing]
* [#point9 GIT]
* [#point10 Code reviews]
* [#point11 Code Editors]
* [#point12 API interface]
* [#point13 GUI guidelines (to be included in the future)]

[=#point1]
== Golden Rule: ==

    '''The committed code should NEVER break the build.'''
    '''If you do break the build - the no. 1 priority is to fix it IMMEDIATELY.'''

[=#point2]
== Coding Standards ==

[=#point3]
Docstring

    When in doubt, the best is to follow PEP 257 (https://www.python.org/dev/peps/pep-0257/). A few rules that are particularly recommended to follow are mentioned below:

    * All modules should normally have docstrings, and all functions and classes exported by a module should also have docstrings.
    * For consistency, always use """triple double quotes""" around docstrings. Use r"""raw triple double quotes""" if you use any backslashes in your docstrings.
    * There are two forms of docstrings: one-liners and multi-line docstrings. One-liners are for really obvious cases. They should really fit on one line.
    * Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.
    * The docstring for a module should generally list the classes, exceptions and functions (and any other objects) that are exported by the module, with a one-line summary of each. (These summaries generally give less detail than the summary line in the object's docstring.) The docstring for a package (i.e., the docstring of the package's {{{ __init__.py }}} module) should also list the modules and subpackages exported by the package.
    * The docstring for a function or method should summarize its behavior and document its arguments, return value(s), side effects, exceptions raised, and restrictions on when it can be called (all if applicable). Optional arguments should be indicated. It should be documented whether keyword arguments are part of the interface.
    * The docstring for a class should summarize its behavior and list the public methods and instance variables. If the class is intended to be subclassed, and has an additional interface for subclasses, this interface should be listed separately (in the docstring). The class constructor should be documented in the docstring for its {{{ __init__ }}} method. Individual methods should be documented by their own docstring.
     
     
    * Models: when adding a new model the .py file starts with an r (for raw) and three sets of quotes to start the doc string and ends with a second set of 3 quotes.
    * Models: don't add/remove the table of parameters which will get autogenerated.
    * Models: add non-math figures to the sasmodels/sasmodels/models/img subdirectory and rename them to something sensible.
    * Models: figure captions should be indented relative to the image tag. This allows us to, for example, change the font of the caption through CSS.
    * Models: don't add the figure number since this will be generated automatically.
    * Models: make sure all references to model parameters are appropriately named, and that the relationship between equation variables and model parameters are obvious.

[=#point4]
Naming

    * Use !CamelCase for class names
    * Use box_car with underscores separating words for variable and method names.
    * Use box_car with underscores for namespaces & directories
    * Use {{{ _box_car }}} with a leading underscore for non-public methods and _CamelCase with a leading underscore for non-public classes.
    * Never use {{{ __box_car__ }}} with leading and trailing double underscores
    * Avoid one-letter variable names (unless is acceptable one letter variable name or it is justified by the math in the paper)
    * Don't make long variable names just to keep lint happy:
      {{{
      #!div style="font-size: 80%"
      {{{#!python  
      [(value, key) for key, value in translation_dictionary.items()]
      }}}
      }}}
      is no better than:
      {{{
      #!div style="font-size: 80%"
      {{{#!python  
      [(v, k) for k, v in translation_dictionary.items()]
      }}}
      }}}
      but the following is:
      {{{
      #!div style="font-size: 80%"
      {{{#!python  
      [(latin, english) for english, latin in translation_dictionary.keys()]
      }}}
      }}}
    * Models: for adding a new model no capitalization and thus no !CamelCase in model names. If necessary use underscore to separate (i.e. barbell not !BarBell or broad_peak not !BroadPeak)
    * Models: Don't add/Remove “model” in/from the name (i.e. barbell not !BarBellModel)
    * Models: When defining parameters use radius_core, sld_core names not core_radius and core_sld

[=#point5]
Imports

    * Import only one module per line:
      {{{
      #!div style="font-size: 80%"
      {{{#!python
      import sascalc
      import sasmodels
      }}}
      }}}
      not
      {{{
      #!div style="font-size: 80%"
      {{{#!python
      import sasmodels, sascalc
      }}}
      }}}
    * It is OK to import multiple classes and/or methods from the same module on the same line:
      {{{
      #!div style="font-size: 80%"
      {{{#!python      
      from sasmodels import barbell, cylinder
      }}}
      }}}
    * Avoid importing * from any module. This is a typically a waste of time and memory. This leads to bugs as well when for example you have "from math import *" and "from numpy import *" in the same file and then decide to reorder them.  It also makes for harder to understand code since you don't know where the symbols are coming from.
    * {{{ from module import *}}} is acceptable in {{{ __init__.py }}} if {{{ module.__all__ }}} defines the set of names being exported.

    * Imports should be grouped in the following order:
        1. standard library imports
        2. related third party imports
        3. local application/library specific imports
    * Put a blank line between each group of imports.

[=#point6]
Exception Handling

    * Use if and try/except block to test for errors that could happen in normal program operation
    * If you catch an exception and all you do is calling pass or print, you have not dealt with the error properly
    * Exceptions should not be used for flow control, unless there is a very good reason for it
    * Error codes from a method should never be returned - use exceptions
    * Never use a blank except: statement since it interferes with !KeyboardInterrupt.  At a minimum, use except Exception:.

[=#point7]
Coding Style

When in doubt - follow PEP 8 (https://www.python.org/dev/peps/pep-0008/)

Indentation

    * Use 4 spaces to indent, not tabs.
    * Indent 4 spaces per nested level.

Spaces in Expressions & Statements

    * Do not put spaces around parentheses, brackets, or curly braces.
    * Do not put spaces before commas, colons, or semicolons.
    * Put one space around operators.
    * However one can use no spaces with operators of higher priority, e.g., {{{ y = a*x**2 + b*x + c}}}.

Miscellaneous

    * Limit lines to a maximum of 79 characters.
    * Do not put semicolons at the end of lines.
    * Avoid using magic numbers
    Use:
    {{{
    #!div style="font-size: 80%"
    {{{#!python        
    max_bins = 7
    if i < max_bins:
    }}}
    }}}
    Not:
    {{{
    #!div style="font-size: 80%"
    {{{#!python        
    if i < 7
    }}}
    }}}
Comments in the code

    * Comments should usually be complete sentences.
    * Use block comments to talk about the code immediately following the comments.
    * Use inline comments (sparingly) to clarify confusing lines.

[=#point8]
== Testing ==

    * Unit tests should test one method only. This allows you to easily identify what failed if the test fails.
    * Unit tests should not be coupled together, therefore one unit test CANNOT rely on another unit test having completed first.
    * Unit tests should use small and simple data sets.
    * Tests should be fast, ideally really fast - certainly not more than a few seconds.
    * Untestable code is a code-smell, if you can't get the code under test it probably needs refactoring.
    * Weight your testing to be destructive rather than demonstrative. Destructive tests are more efficient in finding bugs.
    * Use long and descriptive names for testing functions. The reason is testing functions are never called explicitly. square() or even sqr() is ok in running code, but in testing code you would have names such as test_square_of_number_2(),test_square_negative_number(). These function names are displayed when a test fails, and should be as descriptive as possible.
    * Attributes of a good test: Fast, Clear, Isolated, Reliable
     
    * Models: Include at least one test for each model and PLEASE make sure that the answer value is correct (i.e. not a random number).
    * Models: Remember to test a model for critical values (e.g. q=0)

[=#point9]
== GIT ==

    * Remember to add files using git add before committing your changes.
    * Have a self-explanatory commit message. "Merged with master" is not sufficient. It is a good practice to do a git log before pushing to master and check for the top message. git commit --amend can be used to fix commit message if we are not satisfied with it.  
    Using git branches is encouraged when developing new features or implementing significant changes. It is also a good habit to regularly merge you branch with master as well as push it Github (as a backup).
    * When merging a branch with multiple commits use --squash or interactively rebase before pushing to master.

[=#point10]
== Code Reviews ==

    * Code reviews by another developer are encouraged for minor functionality and required for major changes
    * When developing in the branch it is easy to ask for pull requests. One can also test your code easily by pulling your branch and not messing up with master branch
    * Follow the standard practices:
        1. Review no more than 400 lines of code at a time
        2. Take your time. Inspection rates should be under 500 LOC per hour
        3. Don't review code for more than 90 minutes at a time
        4. Authors should annotate source code before the review. (Annotations guide the reviewer through the changes, showing which files to look at first and defending the reason behind each code modification. Annotations should be directed at other reviewers to ease the process and provide more depth in context)
        5. Use checklists - they substantially improve results for both authors and reviewers (It's very likely that each person on your team makes the same 10 mistakes over and over. Omissions in particular are the hardest defects to find because it's difficult to review something that isn't there. Checklists are the most effective way to eliminate frequently made errors and to combat the challenges of omission finding)
        6. Remember to review related unit tests - they are as important as the code.
        7. Verify that the defects are actually fixed

[=#point11]
== Code Editors ==

    * Tips and tricks of how to make your life easier when developing code with your favorite editor.
    * Example editors used in the group are:
       1. vi
       2. !PyCharm
       3. Visual Studio with PTVS

     * Both !PyCharm and VS allow source level debugging both of self-spawned processes and by attaching to an already running process.

[=#point12]
== API == 

The proposed access to the core functionality should be consistent for all perspectives and straightforward to use.

    * All known input arguments should be set in {{{__init__}}}, during instantiation, with proper checks
    * Additional input data can be set directly through the @property decorator

        {{{
        #!div style="font-size: 80%"
        Getters:
        {{{#!python        
        @property
        def intensity(self):
            return self.wave.intensity
        }}}
        }}}
         
        {{{
        #!div style="font-size: 80%"
        Setters:
        {{{#!python        
        @intensity.setter
        def intensity(self, value):
            self.wave.set_intensity(intensity)
        }}}
        }}}
    * All user accessible input/output data should be available through getters
     {{{ __getattr__}}} and {{{__setattr__}}}} should be avoided.
    * Main functionality should be exposed as a method with easy to understand name, e.g. calculate() and should return the results object/structure
    * When defining the interface, type hints are an important part of the definition. Type hints are described in PEP-484 https://www.python.org/dev/peps/pep-0484 and should be added for all API methods.

    {{{
    #!div style="font-size: 80%"
    {{{#!python            
    def fit(self, msg_q=None, q=None, handler=None, curr_thread=None, ftol=1.49012e-8, reset_flag=False):
           # type: (CalcThread.queue, CalcThread.queue, FitHandler, CalcThread, float, bool) -> Union[CalcThread.queue, FResult]
    }}}
    }}}

    * In cases where several pieces of information are calculated at the same time, a list of outputs, a dictionary, or an output object that contains the computed data should be returned
    * Error codes from a method should never be returned - use exceptions