Code style and conventions

[MichaelClerx]

Do we have any style guides or conventions we're following for Python code?

For example, I saw the fc code uses camelcase for methods (rather than underscore separated)

I'm also a ~fossil~ fan of the 79 char line width, as it means I can have a terminal next to my editor window on the same screen :D

I saw we're using flake8 in some places, which is great (although I usually end up disabled 5-10 of its sillier rules)

[jonc125]

The old fc code follows the Chaste coding conventions since it was part of Chaste. For new Python code I think we should follow PEP8 by default. I tend to use at least flake8 and isort. The pylibrary cookiecutter runs

    python setup.py check --strict --metadata --restructuredtext
    check-manifest {toxinidir}
    flake8 src tests setup.py
    isort --verbose --check-only --diff --recursive src tests setup.py

(see eg https://github.com/SilverLabUCL/PySilverLabNWB)

[MichaelClerx]

Great! Do we have a preference for coverage tools? We've been using codecov for Pints which is usually fine

[jonc125]

That's what the front-end is using too.

[MichaelClerx]

Cool! I also quite like having a CONTRIBUTING.md file, that tells you how to use the project infra. Bit of effort to write and keep up to date, but certainly a nice thing for bigger projects

[jonc125]

I'm now setting up code style checks on the main WebLab repo, and it'd be good to have consistent settings across all repos.

I suggest something like the following; any objections please shout :)

Travis setup:

script:
  - pytest --cov=weblab --cov-config=weblab/.coveragerc weblab
  - flake8 weblab
  - isort --verbose --check-only --diff --recursive weblab
after_success:
  - codecov

Tool configuration in setup.cfg in the project root:

[flake8]
max-line-length = 120
ignore =
    W391  # allow empty line at end of file
    W503  # break before binary operator - allow either style
    W504  # break after binary operator - allow either style
exclude =
    # whatever is relevant for the repo here

[isort]
line_length = 100
multi_line_output = 3
# ^ Vertical Hanging Indent
force_grid_wrap = 4
# ^ Don't allow more than 3 imports on a single line
lines_after_imports = 2
order_by_type = True  # Or False, not a strong opinion here!
include_trailing_comma = True
force_single_line = False
default_section = FIRSTPARTY

known_first_party = # repo-dependent
skip = # repo-dependent
not_skip = # repo-dependent

[MichaelClerx]

I prefer leaving the line width at 79

[jonc125]

I prefer leaving the line width at 79

I'm going to overrule you on that one :)

[MichaelClerx]

Other flake8 exceptions I've had/still have in other projects:

# Block comment should start with '# '
# Not if it's a commented out line
E265,

# "Do not assign a labmda expression, use a def".
# T'is a silly rule.
E731,

# Ambiguous variable names
# It's absolutely fine to have i and I
E741,

# List comprehension redefines variable
# Re-using throw-away variables like `i`, `x`, etc. is a Good Idea
F812,

# Undefined name
# Good rule, but the check doesn't support dynamically created variables
F821,

# Blank line at end of file
# These can increase readability (in many editors)
W391,

# Binary operator on new line
# This is now advised in pep8
W503,

# Line break after binary operator
W504,

# Invalid escape sequence
# These happen all the time in latex parts of docstrings,
# e.g. \sigma
W605,

# Local variable assigned to but never used
# Good rule, but getting a false positive on travis with flake 3.6.0
F841,

[MichaelClerx]

I'm going to overrule you on that one :)

OK! But I just want to say this is a really good rule. Even on large laptops (which fewer people are carrying around) with letters resembling 10-12pt in a book, you can only fit 2 editor windows / 1 editor + 1 console side by side with about ~85 chars (console font, not Times) per line. Long lines means we need massive screens or tiny letters. It's not just a remnant from the terminal-connected-to-a-mainframe days!

[jonc125]

W605 actually caught a real issue for me yesterday! If you want LaTeX in docstrings easily you can make them raw strings instead (r"""..."""). For the others, perhaps we should consider adding them only if we hit those particular issues on a project; certainly they're not coming up on the WebLab repo yet.

On line width, I can't get two windows side-by-side on my laptop even with 80 char line length. So I tend to split the screen horizontally instead, and hence allowing lines to be longer wastes less screen real estate. And in the office we have 2 large screens attached. So these things are always going to be personal preference to some extent.

[jonc125]

I've created https://github.com/ModellingWebLab/cellmlmanip/pull/66 in an effort to demonstrate 'standard' testing for one of our library packages. If it looks good, we can replicate in the other repos.