Add pre commit configuration (take 2)

[hjoliver]

Supersede #3467

These changes partially address cylc/cylc-admin#64

This is @sgaist's branch rebased onto master, with some superficial style changes reverted in setup.py for a minimal diff with current master (so I can investigate why pycodestyle is failing on the CI platform on that branch).

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 (coding style validation only).
• No change log entry required (invisible to users).
• No documentation update required.

• Created an issue at cylc-flow conda-forge repository with version changes (if you changed dependencies in setup.py, see recipe/meta.yaml).
• No dependency changes.

[hjoliver]

(same pycodestyle problem here ... maybe it is flake8 failing - it runs pycodestyle, but the new flake8 config overrides tox.ini??)

[hjoliver]

Ah, that must be it. Added the old tox.ini warnings to the new flake8 config, and the tests are passing.

[hjoliver]

Last commit removes pycodestyle from the Actions workflow, since flake8 runs pycodestyle anyway.

[kinow]

Installed as per CONTRIBUTING.md instructions. Tested running the pre-commit command
directly first. No issues.

(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ pre-commit run
[INFO] Initializing environment for https://github.com/timothycrosley/isort.
[INFO] Initializing environment for https://github.com/psf/black.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/PyCQA/flake8.
[INFO] Initializing environment for https://github.com/PyCQA/bandit.
[INFO] Installing environment for https://github.com/timothycrosley/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/psf/black.
[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/flake8.
[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...
isort................................................(no files to check)Skipped
black................................................(no files to check)Skipped
Check python ast.....................................(no files to check)Skipped
Check for case conflicts.............................(no files to check)Skipped
Trim Trailing Whitespace.............................(no files to check)Skipped
Fix End of Files.....................................(no files to check)Skipped
Debug Statements (Python)............................(no files to check)Skipped
Check for added large files..........................(no files to check)Skipped
Check docstring is first.............................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
bandit...............................................(no files to check)Skipped

Then edited a file, re-run.

(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ git diff
diff --git a/cylc/flow/network/server.py b/cylc/flow/network/server.py
index e446bd4a5..21e014e48 100644
--- a/cylc/flow/network/server.py
+++ b/cylc/flow/network/server.py
@@ -45,8 +45,6 @@ def expose(func=None):
     """Expose a method on the sever."""
     func.exposed = True
     return func
-
-
 def filter_none(dictionary):
     """Filter out `None` items from a dictionary:
 
(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ pre-commit run 
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/kinow/.cache/pre-commit/patch1608074742.
isort................................................(no files to check)Skipped
black................................................(no files to check)Skipped
Check python ast.....................................(no files to check)Skipped
Check for case conflicts.............................(no files to check)Skipped
Trim Trailing Whitespace.............................(no files to check)Skipped
Fix End of Files.....................................(no files to check)Skipped
Debug Statements (Python)............................(no files to check)Skipped
Check for added large files..........................(no files to check)Skipped
Check docstring is first.............................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
bandit...............................................(no files to check)Skipped
[INFO] Restored changes from /home/kinow/.cache/pre-commit/patch1608074742.

Committed the badly-formatted file.

(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ git commit -am 'test'
isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing /home/kinow/Development/python/workspace/cylc-flow/cylc/flow/network/server.py

black....................................................................Failed
- hook id: black
- files were modified by this hook

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

Check python ast.........................................................Passed
Check for case conflicts.................................................Passed
Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Debug Statements (Python)................................................Passed
Check for added large files..............................................Passed
Check docstring is first.................................................Passed
Check Yaml...........................................(no files to check)Skipped
flake8...................................................................Passed
bandit...................................................................Passed

Looks like black automagically fixed it. But also ended up fixing a lot of other stuff.
Which was not part of my commit.

(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ git status
On branch pr-3999
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   cylc/flow/network/server.py

no changes added to commit (use "git add" and/or "git commit -a")
(venv) kinow@ranma:~/Development/python/workspace/cylc-flow$ git diff
diff --git a/cylc/flow/network/server.py b/cylc/flow/network/server.py
index e446bd4a5..df5b44d77 100644
--- a/cylc/flow/network/server.py
+++ b/cylc/flow/network/server.py
@@ -20,24 +20,26 @@ from queue import Queue
 from textwrap import dedent
 from time import sleep
 
-from graphql.execution.executors.asyncio import AsyncioExecutor
 import zmq
+from graphql.execution.executors.asyncio import AsyncioExecutor
 
 from cylc.flow import LOG
-from cylc.flow.network import encode_, decode_, ZMQSocketBase
+from cylc.flow.data_messages_pb2 import PbEntireWorkflow
+from cylc.flow.data_store_mgr import DELTAS_MAP
+from cylc.flow.network import ZMQSocketBase, decode_, encode_
 from cylc.flow.network.authorisation import authorise
 from cylc.flow.network.graphql import (
-    CylcGraphQLBackend, IgnoreFieldMiddleware, instantiate_middleware
+    CylcGraphQLBackend,
+    IgnoreFieldMiddleware,
+    instantiate_middleware,
 )
 from cylc.flow.network.resolvers import Resolvers
 from cylc.flow.network.schema import schema
-from cylc.flow.data_store_mgr import DELTAS_MAP
-from cylc.flow.data_messages_pb2 import PbEntireWorkflow
 
 # maps server methods to the protobuf message (for client/UIS import)
 PB_METHOD_MAP = {
-    'pb_entire_workflow': PbEntireWorkflow,
-    'pb_data_elements': DELTAS_MAP
+    "pb_entire_workflow": PbEntireWorkflow,
+    "pb_data_elements": DELTAS_MAP,
 }
 
 
@@ -60,9 +62,7 @@ def filter_none(dictionary):
 
     """
     return {
-        key: value
-        for key, value in dictionary.items()
-        if value is not None
+        key: value for key, value in dictionary.items() if value is not None
     }
 
 
@@ -125,19 +125,23 @@ class SuiteRuntimeServer(ZMQSocketBase):
 
     """
 
-    def __init__(self, schd, context=None, barrier=None,
-                 threaded=True, daemon=False):
-        super().__init__(zmq.REP, bind=True, context=context,
-                         barrier=barrier, threaded=threaded, daemon=daemon)
+    def __init__(
+        self, schd, context=None, barrier=None, threaded=True, daemon=False
+    ):
+        super().__init__(
+            zmq.REP,
+            bind=True,
+            context=context,
+            barrier=barrier,
+            threaded=threaded,
+            daemon=daemon,
+        )
         self.schd = schd
         self.suite = schd.suite
         self.public_priv = None  # update in get_public_priv()
         self.endpoints = None
         self.queue = None
-        self.resolvers = Resolvers(
-            self.schd.data_store_mgr,
-            schd=self.schd
-        )
+        self.resolvers = Resolvers(self.schd.data_store_mgr, schd=self.schd)
         self.middleware = [
             IgnoreFieldMiddleware,
         ]
@@ -168,10 +172,10 @@ class SuiteRuntimeServer(ZMQSocketBase):
         Overwrites Base method.
 
         """
-        LOG.debug('stopping zmq server...')
+        LOG.debug("stopping zmq server...")
         self.stopping = True
         if self.queue is not None:
-            self.queue.put('STOP')
+            self.queue.put("STOP")
 
     def _listener(self):
         """The server main loop, listen for and serve requests."""
@@ -179,7 +183,7 @@ class SuiteRuntimeServer(ZMQSocketBase):
             # process any commands passed to the listener by its parent process
             if self.queue.qsize():
                 command = self.queue.get()
-                if command == 'STOP':
+                if command == "STOP":
                     break
                 raise ValueError('Unknown command "%s"' % command)
 
@@ -191,7 +195,7 @@ class SuiteRuntimeServer(ZMQSocketBase):
                 # thread to stop
                 continue
             except zmq.error.ZMQError as exc:
-                LOG.exception('unexpected error: %s', exc)
+                LOG.exception("unexpected error: %s", exc)
                 continue
 
             # attempt to decode the message, authenticating the user in the
@@ -205,8 +209,8 @@ class SuiteRuntimeServer(ZMQSocketBase):
             else:
                 # success case - serve the request
                 res = self._receiver(message)
-                if message['command'] in PB_METHOD_MAP:
-                    response = res['data']
+                if message["command"] in PB_METHOD_MAP:
+                    response = res["data"]
                 else:
                     response = encode_(res).encode()
                 # send back the string to bytes response
@@ -227,19 +231,22 @@ class SuiteRuntimeServer(ZMQSocketBase):
         """
         # determine the server method to call
         try:
-            method = getattr(self, message['command'])
-            args = message['args']
-            args.update({'user': message['user']})
-            if 'meta' in message:
-                args['meta'] = message['meta']
+            method = getattr(self, message["command"])
+            args = message["args"]
+            args.update({"user": message["user"]})
+            if "meta" in message:
+                args["meta"] = message["meta"]
         except KeyError:
             # malformed message
-            return {'error': {
-                'message': 'Request missing required field(s).'}}
+            return {"error": {"message": "Request missing required field(s)."}}
         except AttributeError:
             # no exposed method by that name
-            return {'error': {
-                'message': 'No method by the name "%s"' % message['command']}}
+            return {
+                "error": {
+                    "message": 'No method by the name "%s"'
+                    % message["command"]
+                }
+            }
 
         # generate response
         try:
@@ -248,16 +255,23 @@ class SuiteRuntimeServer(ZMQSocketBase):
             # includes incorrect arguments (TypeError)
             LOG.exception(exc)  # note the error server side
             import traceback
-            return {'error': {
-                'message': str(exc), 'traceback': traceback.format_exc()}}
 
-        return {'data': response}
+            return {
+                "error": {
+                    "message": str(exc),
+                    "traceback": traceback.format_exc(),
+                }
+            }
+
+        return {"data": response}
 
     def register_endpoints(self):
         """Register all exposed methods."""
-        self.endpoints = {name: obj
-                          for name, obj in self.__class__.__dict__.items()
-                          if hasattr(obj, 'exposed')}
+        self.endpoints = {
+            name: obj
+            for name, obj in self.__class__.__dict__.items()
+            if hasattr(obj, "exposed")
+        }
 
     @authorise()
     @expose
@@ -278,8 +292,9 @@ class SuiteRuntimeServer(ZMQSocketBase):
         """
         if not endpoint:
             return [
-                method for method in dir(self)
-                if getattr(getattr(self, method), 'exposed', False)
+                method
+                for method in dir(self)
+                if getattr(getattr(self, method), "exposed", False)
             ]
 
         try:
@@ -287,9 +302,9 @@ class SuiteRuntimeServer(ZMQSocketBase):
         except AttributeError:
             return 'No method by name "%s"' % endpoint
         if method.exposed:
-            head, tail = method.__doc__.split('\n', 1)
+            head, tail = method.__doc__.split("\n", 1)
             tail = dedent(tail)
-            return '%s\n%s' % (head, tail)
+            return "%s\n%s" % (head, tail)
         return 'No method by name "%s"' % endpoint
 
     @authorise()
@@ -311,7 +326,7 @@ class SuiteRuntimeServer(ZMQSocketBase):
                 request_string,
                 variable_values=variables,
                 context={
-                    'resolvers': self.resolvers,
+                    "resolvers": self.resolvers,
                 },
                 backend=CylcGraphQLBackend(),
                 middleware=list(instantiate_middleware(self.middleware)),
@@ -320,27 +335,40 @@ class SuiteRuntimeServer(ZMQSocketBase):
                 return_promise=False,
             )
         except Exception as exc:
-            return 'ERROR: GraphQL execution error \n%s' % exc
+            return "ERROR: GraphQL execution error \n%s" % exc
         if executed.errors:
             errors = []
             for error in executed.errors:
-                if hasattr(error, '__traceback__'):
+                if hasattr(error, "__traceback__"):
                     import traceback
-                    errors.append({'error': {
-                        'message': str(error),
-                        'traceback': traceback.format_exception(
-                            error.__class__, error, error.__traceback__)}})
+
+                    errors.append(
+                        {
+                            "error": {
+                                "message": str(error),
+                                "traceback": traceback.format_exception(
+                                    error.__class__, error, error.__traceback__
+                                ),
+                            }
+                        }
+                    )
                     continue
-                errors.append(getattr(error, 'message', None))
+                errors.append(getattr(error, "message", None))
             return errors
         return executed.data
 
     @authorise()
     @expose
-    def get_graph_raw(self, start_point_string, stop_point_string,
-                      group_nodes=None, ungroup_nodes=None,
-                      ungroup_recursive=False, group_all=False,
-                      ungroup_all=False):
+    def get_graph_raw(
+        self,
+        start_point_string,
+        stop_point_string,
+        group_nodes=None,
+        ungroup_nodes=None,
+        ungroup_recursive=False,
+        group_all=False,
+        ungroup_all=False,
+    ):
         """Return a textural representation of the suite graph.
 
         .. warning::
@@ -392,12 +420,14 @@ class SuiteRuntimeServer(ZMQSocketBase):
         """
         # Ensure that a "None" str is converted to the None value.
         return self.schd.info_get_graph_raw(
-            start_point_string, stop_point_string,
+            start_point_string,
+            stop_point_string,
             group_nodes=group_nodes,
             ungroup_nodes=ungroup_nodes,
             ungroup_recursive=ungroup_recursive,
             group_all=group_all,
-            ungroup_all=ungroup_all)
+            ungroup_all=ungroup_all,
+        )
 
     # UIServer Data Commands
     @authorise()

I edited the same file, this time just appending "OK" to an existing pydocs documentation block. There wasn't any formatting issues this time. But again the whole code was formatted with isort, black, etc.

@hjoliver I think this will produce pull requests with a lot more changes than what the OP intended to. Should we tweak the settings a bit to be more lenient and reduce the changes with each PR, or should we use as-is, to slowly improve the code base?

[hjoliver]

@hjoliver I think this will produce pull requests with a lot more changes than what the OP intended to. Should we tweak the settings a bit to be more lenient and reduce the changes with each PR, or should we use as-is, to slowly improve the code base?

Good question. I modified and staged scheduler.py to see what pre-commit run would do. Seems to me all the changes are good ones, but yes it would create confusing PRs for a while.

We could consider merging this, then processing everything with flake8 etc. in one PR (or a few), to reset the bar, then carry on with consistent style from that point on.

[oliver-sanders]

Don't think we need this (slow) one if we are using black in its regular mode (it parses into ast anyway).

Also I think pytest would bail anyway?

[hjoliver]

Sounds convincing. Would you agree @sgaist ?

[sgaist]

Agreed, there might also be other hooks that also uses the AST for their work, so it's one that can be dropped in this case.

[oliver-sanders]

This overrides existing flake8 config present in tox.ini.

[hjoliver]

Yes, that's why the fast tests were failing on the original branch, until I added our usual warning ignore settings here instead of the ones @sgaist used.

As per my comments above, pycodestyle (and flake8, which wraps pycodestyle) ignores the tox.ini config on one of my dev platforms, because of inline comments in the file, even though the pycodestyle version claims to be the same.

However, now that I understand the cause I could move those comments, and we could drop this new config. Is there any reason at all to have both?

Annoyingly, it seems tox.ini needs duplicate pycodestyle settings, under [pycodestyle] and [flake8] sections, unless you never run pycodestyle alone. Also, flake8 ignores W503 and W504 by default (but not if you explicitly ignore anything else).

[sgaist]

There's a bit of complexity with all the possible configuration files that can be used. There's also the pyproject that might be used but may not be supported by all the tools.

I have a project which has flake8 in tox.ini, bandit in its own file because they were (might still) not supporting the pyproject.toml nor tox.ini, and the rest is in pyproject.toml. I like the idea of having a maximum of configurations in a single file but that is rather up to the taste of the maintainer(s).

[hjoliver]

(I'm closing this for now as we haven't managed to get it up the priority list, and it's likely to a while yet ... can possibly revisit at a later date).