Implement a linter as part of the test suite

[JakobGM]

I'm a big fan of tool-based formatting, offloading the task of formatting code to automated tools.

Additionally, the code style section in CONTRIBUTING.rst should be codified in the form of some linter's configuration file.

In order to make this as automated as possible, we should pursue the following goals:

• [x] Make all existing code compliant with some set of lint rules. PEP 8 should be a good starting point. We can always add additional rules at some later point in time.
• [x] Fix linting errors in the test suite.
• [x] Decide on which linter(s) to use. flake8 seems to be a good choice here, but more research needs to be done.
• [x] Research good tools for automatic code formating. YAPF seems interesting.
• [x] Integrate an automatic code formatter as a pre-commit hook.
• [x] Integrate linter into Travis-CI.
• [x] Integrate the linter(s) into the pytest test suite.

I have already started the work of complying to the PEP 8 standard everywhere in the code base.

[JakobGM]

d81a8002b8fbba87c69fe08276185f618c150fc7 to 87de731febd65fe1ec418405dd1d1fc23e432b0f has now fixed all flake8 linting errors in application code.

The huge task of passing linting requirements in test code remains. I think we should ignore several linting rules in testing code, such as docstring requirement, and perhaps increase the allowable line-width to 120 character for the test submodule?

[JakobGM]

I have looked at using the pytest plugin pytest-flake8, but it seems that it breaks --runslow by skipping all slow tests even when pytest is passed --runslow. I will check if this is case for a dummy repository, and submit an issue to tholo/pytest-flake8 if so.

[sshashank124]

Looking at the issues for pytest-flake8, I don't think it is the best tool right now since it breaks with the latest version of flake8 (3.5.0). Maybe we should wait for it to be resolved and look at an alternative option in the meantime

hacking could also be useful

Linters comparison: https://blog.sideci.com/about-style-guide-of-python-and-linter-tool-pep8-pyflakes-flake8-haking-pyling-7fdbe163079d

[JakobGM]

So hacking is basically a collection of curated flake8 plugins, right? Yes, that could be useful, making that decision based on commutity standards.

The weird thing is that I did not experience tholo/pytest-flake8/issues/34 when using the plugin, it gave correct errors and passed finally after implementing the commits mentioned above. But anyway, it seems to be a somewhat stale plugin, without any updates on PyPI the last year, so we should probably wait and see.

But I think we need linting as part of CI at some point, or else I will forget about it all the time :stuck_out_tongue:

We could always do something along the lines of:

#.travis.yml
 install:
+  - "pip install flake8"
   - "pip install -r requirements.txt"
+before_script:
+  - "flake8 ."

But that would not work locally...

[sshashank124]

Maybe we can just go with the Travis solution for now and put it on ourselves to keep linting when developing.

In the meantime, we can keep exploring options for auto lint-checking and we could always fallback onto pytest-flake8 if no alternative is found

[JakobGM]

Ok, I will add it to the Travis-CI configuration now.

[sshashank124]

Additionally, the code style section in CONTRIBUTING.rst should be codified in the form of some linter's configuration file.

flake8-commas for trailing commas flake8-quotes for single quote string literals Could not find one for forced keyword arguments although it seems possible through python3 itself as explained here

[JakobGM]

I think I might have been wrong when I said that forced keyword arguments is the correct thing to do, it is too much dependent of the context, function name, and arity. Perhaps I should remove that part of the style guide? It should be done as part of the core language in those cases it makes sense.

And apropo of nothing, at the momemt, there are lots of places in the code base where we have code like this:

def foobar(
    a,
    b,
    c
):
    pass

But YAPF default behaviour is to double indent such arguments (because they think that is more legible, since it distinguishes arguments from the function body) like this:

def foobar(
        a,
        b,
        c
):
    pass

At the moment, it is not possible to configure YAPF to format code like the way we have done it so far. Take a look at this issue: google/yapf/issues/460

Do you think we should use the double indentation style, or should we disable YAPF from touching any function arguments? I'm really used to single indentation, but style should follow convention, and it seems that I'm in the wrong here...

If we decide to double indent, it should not be much work at all, since I can just run YAPF on each file and commit those changes.

[sshashank124]

I prefer single indenting too but I think it would just be simpler to use the double indentation style and have one less thing to worry about. It'd be nice to introduce exceptions to linting/formatting rules as seldomly as possible

For the forced keyword arguments, it is definitely more readable to have so maybe instead of hard-enforcing it, we can leave it in CONTRIBUTING.rst as a suggested (but not enforced) item