search

cylc / cylc-admin

Project planning for the Cylc Workflow Engine.
https://cylc.github.io/cylc-admin/
GNU General Public License v3.0
5 stars 13 forks source link

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

Open kinow opened 5 years ago

kinow commented 5 years ago

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:

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 :+1:

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

kinow commented 5 years ago

(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 commented 5 years ago

I would also suggest:

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

kinow commented 5 years ago

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 commented 5 years ago

You're welcome !

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

kinow commented 5 years ago

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 commented 5 years ago

Is there already a repository ready for that ?

kinow commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

kinow commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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.