[pre-commit] Add configuration file for pre-commit and flake8

[sgaist]

These changes partially address cylc/cylc-admin#64

Requirements check-list

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Does not need tests (This change is related to code styling/validation.).
• No change log entry required (why? e.g. This change concerns only developers).
• No documentation update required.

[sgaist]

I just realised that a documentation update might be in order for developers/new comers.

One thing that must be decided is how the cleanup shall be done.

For example:

• per-file when one should be modified ? -> some may not be changed before a long time
• once on all the code base ? -> might break pull request already submitted.
• other ?

[kinow]

I think linter and unit tests are passing. You can leave the functional tests failures apart @sgaist. They are not deterministic. I will kick Travis a few times until they pass, and try to review this one today too. Thanks!

[kinow]

I think we just need to include pre-commit==1.* with the test dependencies or something similar to the setup.py file @sgaist ?

We are also missing at least a short paragraph in CONTRIBUTING.md to tell users to run pip install -e .[all] in order to actually contribute code. But that can be done separately I believe. I had a peek at the JupyterHub CONTRIBUTING.md to remember what was missing when pre-commit was not found.

Other than that, everything looks good. Here's the output after I added a function to flags.py without adding the proper space first. Note that the command execution took a couple minutes. But this happens only during the first commit.

(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ git commit cylc/flow/flags.py -m 'test'
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/kinow/.cache/pre-commit/patch1578160856.
[INFO] Initializing environment for https://github.com/ambv/black.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/PyCQA/bandit.
[INFO] Installing environment for https://github.com/ambv/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/asottile/reorder_python_imports.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/PyCQA/bandit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted cylc/flow/flags.py
All done! ✨ 🍰 ✨
1 file reformatted.

Reorder python imports...................................................Failed
- hook id: reorder-python-imports
- exit code: 1
- files were modified by this hook

Reordering imports in cylc/flow/flags.py

Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Debug Statements (Python)................................................Passed
Check for added large files..............................................Passed
Check docstring is first.................................................Passed
Flake8...................................................................Passed
Check for case conflicts.................................................Passed
Check that executables have shebangs.................(no files to check)Skipped
Fix requirements.txt.................................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
bandit...................................................................Passed
[INFO] Restored changes from /home/kinow/.cache/pre-commit/patch1578160856.

black fixed my file, and looking at the git diff it is possible to confirm the file has been edited by the linter (I never typed the extra blank line before the test function).

(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ git log -n 1 --patch
commit f75a70e80465ce6e4d977f2e02523d3ec60f2637 (HEAD -> add_pre_commit_configuration)
Author: Bruno P. Kinoshita <[email protected]>
Date:   Sun Jan 5 07:08:51 2020 +1300

    test

diff --git a/cylc/flow/flags.py b/cylc/flow/flags.py
index d66e44f76..3e8002d94 100644
--- a/cylc/flow/flags.py
+++ b/cylc/flow/flags.py
@@ -13,11 +13,13 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
 """Some global flags used in cylc"""
-
 # verbose mode
 verbose = False
 
 # debug mode
 debug = False
+
+
+def test():
+    pass

Nice job @sgaist !

[kinow]

Travis happy, Codacy happy. Codecov can be ignored (small change in a Python file for linter, no tests required) 👍

[sgaist]

@kinow Sorry, I missed the notification.

For adding pre-commit to the test dependencies, will pre-commit be run as part of the test suite ? If so then yes, it makes sense.

The documentation update content will partly depend on the answer to the above.

[kinow]

For adding pre-commit to the test dependencies, will pre-commit be run as part of the test suite ? If so then yes, it makes sense.

It won't run as part of the test suite. Only for the commit hook. I think this is a good point to move it to somewhere else.

The documentation update content will partly depend on the answer to the above.

Maybe we could have a new dependency group dev? With just the pre-commit dependency for now? And include it as part of all.

[sgaist]

@kinow I made the modification you requested.

One thing that still needs be done is to ensure the configuration of black/flake8 matches the style you would like to use within Cylc. For example the line length, the position of the binary operator, etc.

[hjoliver]

meeting: consider replacing this with enforcing flake8 compliance via tests?

[sgaist]

Do you mean run flake8 by hand in a dedicated test ?

[hjoliver]

Do you mean run flake8 by hand in a dedicated test ?

Sorry @sgaist - I'll have to remind myself of the context here. We have project meeting tonight, will try to make a decision on this.

[sgaist]

I have worked on several projects that are currently using tox to run the usual code testing but also have dedicated environment to run flake8 and black checks.

I think it's a tool worth considering.

In any case, both pre-commit and tests can make sense put together. The former for the developer to be able to directly apply cleanups, checks, etc on commit (if not manually run while developing) and the later to ensure it has been done before merging.

[hjoliver]

@sgaist - there was still some minor disagreement in the ranks on whether or not running flake8 in pre-commit is a good thing (e.g. it is common to commit temporary/intermediate changes locally that will likely fail) ... however we decided to go ahead with it in the end. So, thanks for this PR, and apologies for the long delay! Do you want to update the branch? setup.py conflicts now; it's probably an easy fix, but I'll volunteer if you like since the delay was not your fault...

[sgaist]

No problem, I'll update it.

I am wondering if there was not a small misunderstanding on the workflow with pre-commit. It's a tool that must be explicitly installed by the developer in order to be active. You can couple it with code quality tests so you can ensure that if someone does not use pre-commit, they will still have the tests to scream about the standards you want to use.

For example, in a small side project I have worked on, I am using tox for handling the tests and one of the environment does just run code quality checks that include flake8, black, bandit and isort. I also have a pre-commit configuration file so that I have the best of both worlds.

[hjoliver]

It's a tool that must be explicitly installed by the developer in order to be active. You can couple it with code quality tests so you can ensure that if someone does not use pre-commit, they will still have the tests to scream about the standards you want to use.

I was wondering about that but hadn't got around to trying it - thanks for clarifying, it sounds ideal.

[hjoliver]

@sgaist - thanks for you work here, and apologies for the 1-year delay!

I've rebased your branch and made a new PR out of it, to finish it off without bothering you, so I'll close this as superseded. (But your commits will still go in, on the new PR).