Add pre-commit hooks to prevent issues with build tools earlier

[kinow]

Suggested by @sgaist (thanks!). We have pull requests where contributors may spend a long time trying to fix the build (especially around things like 80 characters-lines, and simple unit tests).

It is simply impossible to wait for the functional tests, or even the unit tests I think, as you wouldn't want to wait 1 minute for a git commit ... to finish. So the hook must be kept as simple as possible.

Example of hooks from other projects:

• JupyterHub
  • https://github.com/jupyterhub/jupyterhub/blob/master/.pre-commit-config.yaml
    • order imports (some IDE's, linters, and other tools try to keep them in order)
    • checks the extra line in the file
    • checks that executables have shebangs
    • run flake8
    • etc
• Tornado
  • https://github.com/pandas-dev/pandas/blob/master/.pre-commit-config.yaml
    • also runs flake8, black
    • sorts imports
• SQLAlchemy
  • https://github.com/sqlalchemy/sqlalchemy/blob/master/.pre-commit-config.yaml
    • Just blacks and a custom zimports script

This also means that users contributing to the project are not able to commit without a working environment (well, there's always --no-verify I guess). For JupyterHub, last time I tried to commit something there, it raised an error and asked me to activate my virtualenvironment if I remember well 👍

EDIT: list of projects that would need the pre-commit hooks added:

• cylc/cylc-flow
• cylc/cylc-uiserver
• cylc/cylc-ui
• cylc/cylc-doc
• cylc/cylc-xtriggers
• cylc/cylc-sphinx-extensions

[kinow]

(we don't need to sort imports if others prefer to leave them as they are, though that would be helpful. I have to keep asking my IDE to not touch imports, but for new contributors it may be a bit frustrating)

[sgaist]

I would also suggest:

• bandit
• check-docstring-first
• debug-statements
• check-added-large-files

Just one thing to take into account is that you have to call pre-commit install before the hooks get called automatically.

[kinow]

Just one thing to take into account is that you have to call pre-commit install before the hooks get called automatically.

Funny, I don't remember calling that for JupyterHub at least. Though I probably did, as just noticed they mention it in their CONTRIBUTING.md. Thanks @sgaist!

[sgaist]

You're welcome !

It's also mentioned in pre-commit's usage.

[kinow]

And note to others, I've created it here, as I think this could be applied to all our projects, JS and Python. Perhaps even added/merged to the #63 work.

[sgaist]

Is there already a repository ready for that ?

[kinow]

Is there already a repository ready for that ?

Not yet, I raised that issue last week, but it was after we had a team meeting on Riot, and @hjoliver was also on leave. If there are no objections from the other devs we can create that repository.

I'm widely known for not being good with names, but I think it could be something like cylc/cylc-repo (merely copying the octo-repo from GH docs), or cylc/cylc-project-template.... any suggestions?

[sgaist]

I was wondering, will these be:

1. "full blown projects"
2. "cylc modules"
3. "Others"

If number one, then the name seems fitting.
If number two, then cylc-module-template would likely be more clear.

On a related note, will these additional projects mandatorily provide one ore more new command(s) for cylc to use ?

If so, then structuring them with click would likely make sense since it provides an infrastructure that allows for new package to plug in the main command.

[kinow]

I was wondering, will these be:

Not quite sure. I think the more generic the merrier here. The next repository we may get could be one for Singularity containers. Or rename the cylc-docker to something like cylc-containers, not sure. So not necessarily a cylc module, more likely to be either #1 or #3?

On a related note, will these additional projects mandatorily provide one ore more new command(s) for cylc to use ?

Not really, they could be other JS components (I have one in mind, but for future, a JupyterLab widget to display workflow statuses).

But there will be likely one for a new command/subcommand. To fix #38, probably a module that provides cylc test-battery, only for build & development (or users that want to run the test-battery I guess).

If so, then structuring them with click would likely make sense since it provides an infrastructure that allows for new package to plug in the main command.

This could be a very convincing argument pro-click. Planning to look at your branch this week, but as for cylc vs. argparse, as I said, I'm biased for using click for all my projects :) so I'm already partially sold for that (unless the amount of changes to move to click is really really hard/risky)

[sgaist]

I suggest moving things back to #63 as this is currently more about creating a template repository than adding pre-commit.

[kinow]

I suggest moving things back to #63 as this is currently more about creating a template repository than adding pre-commit.

I'd leave this issue open and add a list of projects that need the pre-commit hooks @sgaist. I feel like the project template could take a bit longer to get resolved. But we can start working on the pre-commit hook right now, and use this issue to keep track of which projects were updated.

[kinow]

Added a list of what needs to be done, now we just need to set up the first project (probably cylc-ui or cylc-uiserver as their build has no flaky tests and ends quickly), prepare a PR, get others to agree, and then replicate to all other projects.

Thoughts?

[hjoliver]

I think it's too early (since Python 3 migration, setup.py, and splitting the original main repo up...) to say if we'll need separate "cylc modules" template, or if new projects would "mandatorily provide one or more new commands" (probably not) - so yeah best a single generic template for now. We can always adjust later if needed.

[sgaist]

Just in case, pre-commit is not only for python.

There's a list of hooks:

https://pre-commit.com/hooks.html

that also shows some stuff available for e.g. JS, markdown etc.

And depending on what you would like to have linted/checked/etc. One can also create it's own local hooks.