Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pre-commit actions #33

Closed
alexander-novo opened this issue Dec 3, 2024 · 1 comment · Fixed by #68
Closed

Add pre-commit actions #33

alexander-novo opened this issue Dec 3, 2024 · 1 comment · Fixed by #68
Assignees
Labels
development Features/Tools related to development of GridKit, rather than use as a library. enhancement New feature or request

Comments

@alexander-novo
Copy link
Collaborator

We should add clang-format and any linters to pre-commit actions. There are a variety of ways that we can implement these actions, but these are the three ways forward I can see:

  1. We make our own pre-commit hooks in Git's hook system.
  • Pros: No extra dev dependencies than the programs we actually want to use in the script
  • Cons: Git hooks aren't pushed, so we must distribute them in the repository along with a script that installs them. As well, any scripts we wish to support would limit the developer environment to being able to run those scripts (i.e. we may force developers to use bash)
  1. We use the pre-commit python framework, along with pre-made hooks for whatever actions we want
  • Pros: Very little development effort - it seems like there are pre-developed hooks for a lot of what we want (formatting, linting, warning on committing to a protected branch, etc.). All of the dependencies for hooks will be automatically installed. Very cross-platform.
  • Cons: Potentially very many dependencies, since each hook will be an extra dependency, and each of them may have sub-dependencies, even if the hook is simple and the software needed for the hook is already pre-installed (if clang-format, for instance, is already installed, a clang-format hook would install a separate version of clang-format specifically for the hook). Python also is added as a dependency.
  1. We use the pre-commit python framework, and make our own hooks.
  • Pros: Few dependencies, ease of use
  • Cons: Python dependency. The pre-commit framework doesn't seem to be designed around small inline hooks, and rather expects you to install hooks on the fly from a separate repository. If we want to make our own hooks, it may be necessary to write some amount in Python

It's also worth noting that all of the above options are entirely opt-in... since Git hooks don't get pushed to the repository, there must be some additional effort put in by the developer to enable these hooks, so we can't depend on developers enabling these hooks. These hooks should only be considered, then, to be warnings or handy conveniences. If we want actual enforcement, then we need to actually put them in CI (#25)

In my opinion, I'm leaning towards option 2 (or 3 if we don't want extra dependencies) since I think we're going to be adding Python as somewhat of a dev dependency for spack anyway (#24).

@alexander-novo alexander-novo added development Features/Tools related to development of GridKit, rather than use as a library. enhancement New feature or request labels Dec 3, 2024
@pelesh
Copy link
Collaborator

pelesh commented Jan 18, 2025

Thanks @alexander-novo for the detailed analysis. I would go with option 2. Developers would need to install Python utilities but these would not be GridKit dependencies. Users would be able to build GridKit and include it in their applications without Python, pre-commit or other tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Features/Tools related to development of GridKit, rather than use as a library. enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants