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 support for the pre-commit project #44

Closed

Conversation

aharrison24
Copy link

  • Adds a config file that allows users of https://pre-commit.com
    to easily add a cmake-format pre-commit hook to their git project.

@cheshirekow
Copy link
Owner

Are you associated with this pre-commit project at all? It's new to me. It strikes me as odd to rely on every up-stream project to provide supporting configuration files in their repository. Looking at what is being specified, is it not possible for users of this pre-commit thing to specify that information in their own .pre-commit-config.yaml. Also, how does pre-commit know what to run? Does language: python and entry cmake-format mean "Add this directory to the python path of the environment and then launch python with -m cmake_format"? Or does it execute the entry: cmake-format command. If so, how does it find that command? Does it automatically run setup.py for python packages?

@mathstuf
Copy link

Also curious what it does when a project already has a pre-commit hook (e.g., LFS). Though that's more a question for upstream :) .

@aharrison24
Copy link
Author

I'm not associated with the project, but I have recently started to use it for automatic linting and formatting on my python and c++ projects with a lot of success. Essentially it makes it trivially easy to set up pre-commit hooks. Most importantly it handles automatic downloading of dependencies into a virtual environment.

A good example is these setup instructions to get the 'black' python formatter running as a pre-commit hook:
https://github.com/ambv/black#version-control-integration
Note that at no point do you have to actually download or install 'black' yourself.

The way this works is that somebody maintains a git repository containing configuration details for each linter. An example is this one for clang-format:
https://github.com/doublify/pre-commit-clang-format

Rather than having this stuff in separate repos, it's convenient if the upstream linter project can own the pre-commit config themselves. That allows the maintainer of the linter to be able to update things like the required python version or necessary command line arguments. It also gives an obvious place for people to obtain the pre-commit config, rather than having to search for random repos on github.

@mathstuf When you install pre-commit, the default behaviour is for it to run the existing hook scripts alongside the pre-commit ones.
https://pre-commit.com/#pre-commit-install

@asottile
Copy link

asottile commented Jun 5, 2019

👋 upstream maintainer here!

@cheshirekow the small hidden metadata file makes it easy for any consumer to add a small amount of yaml to associate with your repository. you don't need to provide installation instructions either as pre-commit knows based on the language key how to install your package (and in an isolated environment so it can't be interfered with!). putting the metadata in the tool's repo (this one) has some benefits, notably that each consumer will get consistent settings without having to copy paste a bunch of boilerplate.

Happy to answer any other questions about this -- the docs for what each of the fields mean are documented here

@iconmaster5326
Copy link

iconmaster5326 commented Sep 26, 2019

I use pre-commit in a project of mine that already uses CMake, and I'm pleased to see pre-commit support is in the works. My CMake files are in desperate need of auto-formatting.

- Adds a config file that allows users of https://pre-commit.com
  to easily add a cmake-format pre-commit hook to their git project.
@aharrison24
Copy link
Author

@qibint I saw there was some recent interest in this PR, so I rebased it on to master a couple of days ago. I believe it's now in a fit state to be merged again.

@cheshirekow
Copy link
Owner

I'm still not super convinced about the whole pre-commit project idea. Seems like Yet Another Bad Build System to me. If others are using it and adding a file to this repo is helpful to them then I'm happy to do so. Can we get it out of the root though? Like the vscode_extension?

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved
.. code:: yaml

repos:
- repo: https://github.com/cheshirekow/cmake_format.git
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, it sounds like pre-commit will download from github and install with setup.py. I would prefer users install from pypi.org as the canonical distributions. Is that possible to configure?

cmake_format/doc/README.rst Outdated Show resolved Hide resolved
@aharrison24
Copy link
Author

@asottile can certainly provide better information, but I've looked in to this and I agree with you that pre-commit does appear to expect .pre-commit-hooks.yaml and setup.py to be in the top-level dir in the repo.

I experimented with putting those files in a subdirectory, but did not have any success. Maybe there is a way, but I didn't find it. @asottile can you comment? Is there an entry we can put in the .pre-commit-config.yaml that will cause pre-commit to look in a sub-directory of the hook repo?

Perhaps a more palatable solution would be for you to have a separate repo in your github account that provides only a pre-commit config for those that want it. This has the advantage that you can provide a custom setup.py for that repo which installs cmake_format via pip.

I've set up my own repo to demonstrate this at
https://github.com/aharrison24/cmake-format-pre-commit.git
You're very welcome to fork it. In fact I'd positively encourage that, as I think it's probably more appropriate that you own it.

@asottile
Copy link

asottile commented Oct 7, 2019

@asottile can certainly provide better information, but I've looked in to this and I agree with you that pre-commit does appear to expect .pre-commit-hooks.yaml and setup.py to be in the top-level dir in the repo.

I experimented with putting those files in a subdirectory, but did not have any success. Maybe there is a way, but I didn't find it. @asottile can you comment? Is there an entry we can put in the .pre-commit-config.yaml that will cause pre-commit to look in a sub-directory of the hook repo?

Perhaps a more palatable solution would be for you to have a separate repo in your github account that provides only a pre-commit config for those that want it. This has the advantage that you can provide a custom setup.py for that repo which installs cmake_format via pip.

I've set up my own repo to demonstrate this at
https://github.com/aharrison24/cmake-format-pre-commit.git
You're very welcome to fork it. In fact I'd positively encourage that, as I think it's probably more appropriate that you own it.

yes that is where it looks and that is only where it looks

@cheshirekow
Copy link
Owner

Ok thanks @aharrison24 and @asottile. I think a separate repo sounds good and thanks for creating the demo. I'll test this out and get reply back to this thread.

@asottile
Copy link

asottile commented Oct 7, 2019

Ok thanks @aharrison24 and @asottile. I think a separate repo sounds good and thanks for creating the demo. I'll test this out and get reply back to this thread.

ideally the file would be put here, this allows it to stay in lock step with releases and drive stars / engagement directly with the repository (instead of having to constantly bounce issues). the file itself is usually very stable and doesn't require changes after initial commit (and if you feel pressured about changes to it, I'm sure either I or one of the others in the thread would be happy to field questions / maintain that file).

the separate repo is kind of a stop-gap hack :S

@cheshirekow
Copy link
Owner

ideally the file would be put here, this allows it to stay in lock step with releases and drive stars / engagement directly with the repository

I think that "ideally", pre-commit would install from pypi. Since it sounds like that is not possible, a separate repo is apparently the only solution. Also, In my opinion, "ideally" pre-commit wouldn't need to to reference some remote git repository just to pull in essentially two lines of yaml config. Seems like "ideally"

repository: https://pypi.org
package: cmake-format
rev: v0.5.5
entry: cmake-format
args: [--in-place]
language: [cmake]

contains all the needed information, without an additional unversioned remote dependency added to the project.

@asottile
Copy link

I understand you don't agree with the design decisions but you don't have to be rude / condescending about it -- I'm merely trying to help <3

pre-commit chooses git as a lowest common denominator as its distribution mechanism. This has a few benefits: packages can easily fork, pre-release versions can be installed immediately. The main reason however is that although pre-commit itself is written in python, it is not a python tool. pre-commit understands and installs tools in many different programming languages.

You also can install directly from pypi, but there are several disadvantages (as listed above) -- a configuration for that would look like:

-   repo: local
    hooks:
    -   id: cmake-format
        name: cmake-format
        entry: cmake-format --in-place
        language: python
        additional_dependencies: [cmake==0.5.5]
        types: [cmake]

and this is the bare minimum configuration necessary to make this work, quite a lot to copy paste :) -- the point of putting the yaml directly in the repository allows users to get sane defaults without needing to configure everything individually -- all you need is (repository, version) pairs essentially and you've already got a good starting place

thanks for reading and hope you have a good day :)

@cheshirekow
Copy link
Owner

I understand you don't agree with the design decisions but you don't have to be rude / condescending about it -- I'm merely trying to help <3

I didn't mean to be condescending 😕. I appreciate the help.

You also can install directly from pypi, but there are several disadvantages (as listed above)

Sorry, I must have missed it. I didn't see any disadvantages listed above... What are they?

-- a configuration for that would look like:

-   repo: local
    hooks:
    -   id: cmake-format
        name: cmake-format
        entry: cmake-format --in-place
        language: python
        additional_dependencies: [cmake==0.5.5]
        types: [cmake]

Wait, does this really work? Then I don't understand what all the extra discussion is about. Let's put this in the documentation and call it a day. No extra repo, no extra file, no additional dependencies.

quite a lot to copy paste :) >

Really, I count three lines more than what is in this pull request. If you're going to copy-paste anything, the difference between 5 lines and 8 seems like nothing to me.

@asottile
Copy link

Wait, does this really work? Then I don't understand what all the extra discussion is about. Let's put this in the documentation and call it a day. No extra repo, no extra file, no additional dependencies.

yes of course it works, but it's a worse user experience

Sorry, I must have missed it. I didn't see any disadvantages listed above... What are they?

(I'll put them in a list so it's easier to digest)

  • easy fork
  • easy pre-release
  • automated updates
  • the consumer doesn't have to understand / worry about the language a tool is written in or the calling construct or flags or etc.
  • direct users / stars / credit to the creating repository via direct attribution

you'd end up with:

-   repo: https://github.com/cheshirekow/cmake_format
    rev: ...
    hooks:
    -   id: cmake-format

and the small metadata file in your repo would handle giving the consumer reasonable defaults (that they can override as necessary)

@qibint
Copy link

qibint commented Oct 14, 2019

Wait, does this really work? Then I don't understand what all the extra discussion is about. Let's put this in the documentation and call it a day. No extra repo, no extra file, no additional dependencies.

yes of course it works, but it's a worse user experience

Sorry, I must have missed it. I didn't see any disadvantages listed above... What are they?

(I'll put them in a list so it's easier to digest)

  • easy fork
  • easy pre-release
  • automated updates
  • the consumer doesn't have to understand / worry about the language a tool is written in or the calling construct or flags or etc.
  • direct users / stars / credit to the creating repository via direct attribution

you'd end up with:

-   repo: https://github.com/cheshirekow/cmake_format
    rev: ...
    hooks:
    -   id: cmake-format

and the small metadata file in your repo would handle giving the consumer reasonable defaults (that they can override as necessary)

"additional_dependencies: [cmake==0.5.5]" - is the dependency for cmake or cmake-format?

@asottile
Copy link

ah I made a typo above, it should be additional_dependencies: [cmake-format==0.5.5] -- my b

@qibint
Copy link

qibint commented Oct 14, 2019

ah I made a typo above, it should be additional_dependencies: [cmake-format==0.5.5] -- my b

It works! Basically, cmake-format will install from the pypi.org because of this dependency config and will make the cmake-format a local executable. It's tricky but I like this kind of usage:)

@cheshirekow
Copy link
Owner

yes of course it works, but it's a worse user experience

Sorry, I'm not understanding how it's a worse user experience, can you elaborate? As far as I can tell the differences are exactly this:

  • Option A:
    • copy/paste 5 lines of config
    • pre-commit transparently downloads a release package from github and installs it with pip
  • Option B:
    • copy/paste 8 lines of config
    • pre-commit transparently downloads nothing and installs the package by name with pip (which downloads a wheel from pypi.org)

I don't really see a user experience difference.

Regarding your list of (dis)advantages:

easy fork

i would argue that "no fork" is the easiest fork... unless you mean that "the user can easily find the upstream source so they can modify it / contribute to it" in which case, I would much rather people go to pypi.org and see what the repository link is there. If (when) this repository moves, the pypi entry will get updated with the new public mirror... no-one is left out of the loop.

easy pre-release

seems that if you wanted to use a pre-release you'd have to modify your local config anyway so I'm not seeing how this is any easier/harder with or without the upstream config bits

automated updates

is an anti-feature... changes behavior (at best) or randomly breaks things that previously worked (at worst) and also means that two people working on the same repository at the same time might be executing with different configs

the consumer doesn't have to understand / worry about the language a tool is written in or the calling construct or flags or etc.

If by "worry about" you mean "copy-paste an additional single line of config containing a string specifying the language"... ok I guess. I don't think writing the word "python" in a file is worth optimizing away. And, lacking any other consideration, I think explicit is better than implicit.

direct users / stars / credit to the creating repository via direct attribution

this is a good point, except that in this case I'd much rather people be directed to the distribution repository (pypi.org) not the source repository (github).

So seems to me like the configuration you provided above that involves no github reference is most appropriate for cmake-format.

@asottile
Copy link

yes of course it works, but it's a worse user experience

Sorry, I'm not understanding how it's a worse user experience, can you elaborate? As far as I can tell the differences are exactly this:

* Option A:
  
  * copy/paste 5 lines of config
  * pre-commit transparently downloads a release package from github and installs it with pip

* Option B:
  
  * copy/paste 8 lines of config
  * pre-commit transparently downloads nothing and installs the package by name with pip (which downloads a wheel from pypi.org)

I don't really see a user experience difference.

fair, but you also don't use the tool so you wouldn't notice the differences :) hopefully the answers to the questions below can help you understand. But the difference isn't just a few lines of copy paste, it's the need to configure each and every tool. Kind of the power of pre-commit is you can let someone else smarter than you configure the tools for you and then you don't even have to think about the nobs. You can configure a bunch of tools very quickly (black, flake8, etc.) and you're ready to go

Regarding your list of (dis)advantages:

easy fork

i would argue that "no fork" is the easiest fork... unless you mean that "the user can easily find the upstream source so they can modify it / contribute to it" in which case, I would much rather people go to pypi.org and see what the repository link is there. If (when) this repository moves, the pypi entry will get updated with the new public mirror... no-one is left out of the loop.

what I mean here is it's trivial for someone to take cmake-format and work on their own copy and install it from their own repository (internal repo, etc. etc.). they use the functionality provided by git to create commits and reference those without needing to pester upstream to make a release or implement a feature which they do not want to implement

easy pre-release

seems that if you wanted to use a pre-release you'd have to modify your local config anyway so I'm not seeing how this is any easier/harder with or without the upstream config bits

yes you would have to edit your configuration, but how would you in the pypi case? the maintainer would need to make a pre-release release, but in the git case you can simply reference a sha1 of any unreleased commit. basically I can get a bugfix release whenever I want without having to haggle a maintainer to make a release!

automated updates

is an anti-feature... changes behavior (at best) or randomly breaks things that previously worked (at worst) and also means that two people working on the same repository at the same time might be executing with different configs

sorry you're misunderstanding here, I wasn't clear enough >.< pre-commit aims for repeatability and the configuration very much gives you that. The automated updates I'm referring to are through pre-commit autoupdate which knows how to move forward rev: ... based versions. It cannot adjust additional_dependencies because those are (at least to pre-commit) arbitrary strings passed through to the underlying package management tools (they may ~usually be dependencies, but you can also use that field to send (for instance) pip options). Here's an example: pre-commit/pre-commit@dc28922

the consumer doesn't have to understand / worry about the language a tool is written in or the calling construct or flags or etc.

If by "worry about" you mean "copy-paste an additional single line of config containing a string specifying the language"... ok I guess. I don't think writing the word "python" in a file is worth optimizing away. And, lacking any other consideration, I think explicit is better than implicit.

direct users / stars / credit to the creating repository via direct attribution

this is a good point, except that in this case I'd much rather people be directed to the distribution repository (pypi.org) not the source repository (github).

So seems to me like the configuration you provided above that involves no github reference is most appropriate for cmake-format.

an odd choice but that's on you :) can't star / make bug reports from pypi but ok

@cheshirekow
Copy link
Owner

fair, but you also don't use the tool so you wouldn't notice the differences :)

I installed it to try out the experience. Both options appear equal to me.

hopefully the answers to the questions below can help you understand. But the difference isn't just a few lines of copy paste, it's the need to configure each and every tool.

Not sure I follow. cmake-format doesn't appear to need much config for pre-commit so seems to me there isn't much need for an upstream config file. Maybe other projects are different. I don't think cmake-format forgoing a config file precludes other projects from doing so.

Kind of the power of pre-commit is you can let someone else smarter than you configure the tools for you and then you don't even have to think about the nobs. You can configure a bunch of tools very quickly (black, flake8, etc.) and you're ready to go

Seems this remains true even without a config file embedded in this repo.

what I mean here is it's trivial for someone to take cmake-format and work on their own copy and install it from their own repository (internal repo, etc. etc.). they use the functionality provided by git to create commits and reference those without needing to pester upstream to make a release or implement a feature which they do not want to implement

Seems this remains possible by the user changing their config to a git-repo style config. And if they get to this point they clearly need to know what language the tool is written in.

yes you would have to edit your configuration, but how would you in the pypi case? the maintainer would need to make a pre-release release, but in the git case you can simply reference a sha1 of any unreleased commit. basically I can get a bugfix release whenever I want without having to haggle a maintainer to make a release!

I think you would just switch to the git configuration if you wanted to install from sha1. Presumably then you're also willing to install the projects build dependencies in addition to the runtime dependencies, and wait for the project to build itself too. Seems like one would generally prefer to install a distribution artifact rather than from source.

sorry you're misunderstanding here, I wasn't clear enough >.<

Indeed, I was misunderstanding. Maybe in the future pre-commit can learn to understand version numbers for pypi too. In the meantime, it doesn't sound a like a feature worth going the additional-file-in-upstream-repo route.

an odd choice but that's on you :) can't star / make bug reports from pypi but ok

pypi links directly to the upstream:

And when the repository and issue tracker inevitably move, users can find it through the package manager.

@asottile
Copy link

asottile commented Oct 17, 2019

I just recalled another case where the local usage is significantly worse -- monorepos :(

For example:

-   repo: ...
    rev: ...
    hooks:
    -   id: cmake-format
        files: ^proj[abc]/  # this one uses default args
    -   id: cmake-format
        files: ^proj1/
        args: [...]  # specific to proj1
    -   id: cmake-format
        files: ^proj2/
        args: [...]  # specific to proj2

vs

-   repo: local
    hooks:
    -   id: cmake-format
        name: cmake-format
        entry: cmake-format --in-place
        language: python
        additional_dependencies: [cmake-format==0.5.5]
        types: [cmake]
        files: ^proj[abc]/  # this one uses default args
    -   id: cmake-format
        name: cmake-format
        entry: cmake-format --in-place
        language: python
        additional_dependencies: [cmake-format==0.5.5]
        types: [cmake]
        files: ^proj1/
        args: [...]  # specific to proj1
    -   id: cmake-format
        name: cmake-format
        entry: cmake-format --in-place
        language: python
        additional_dependencies: [cmake-format==0.5.5]
        types: [cmake]
        files: ^proj2/
        args: [...]  # specific to proj2

(etc. etc.)

@cheshirekow
Copy link
Owner

I just recalled another case where the local usage is significantly worse -- monorepos :(

Wait, so hook config entries are reusable if they're in a remote repository but not if they're in a local repository? That's weird... why is that?

Maybe someone can create a pull request on https://github.com/pre-commit/pre-commit-hooks for people who feel like it's helpful then.

@asottile
Copy link

I just recalled another case where the local usage is significantly worse -- monorepos :(

Wait, so hook config entries are reusable if they're in a remote repository but not if they're in a local repository? That's weird... why is that?

yep! the repo-based hooks are essentially (repo, rev, hookid) pointers to the backing configuration. then one can then override the defaults as necessary (and repeat as needed). this is ~somewhat mentioned in the documentation (especially around aliasing hooks) on https://pre-commit.com. the local repository is "I don't have a backing repo, I'll write out all the metadata myself" and as such there's nothing to "pointer" to

Maybe someone can create a pull request on https://github.com/pre-commit/pre-commit-hooks for people who feel like it's helpful then.

that repository (moving forward) is intended for language-agnostic, universally useful hooks -- though if this PR is accepted it'll be added to the list of supported hooks via https://github.com/pre-commit/pre-commit.github.io 👍

@mathstuf
Copy link

Are merge keys not supported? That would allow something like this:

.cmake: &cmake
  id: cmake-format
  name: cmake-format
  entry: cmake-format --in-place
  language: python
  additional_dependencies: [cmake-format==0.5.5]
  types: [cmake]

-   repo: local
    hooks:
    -   <<: *cmake
        files: ^proj[abc]/  # this one uses default args
    -   <<: *cmake
        files: ^proj1/
        args: [...]  # specific to proj1
    -   <<: *cmake
        files: ^proj2/
        args: [...]  # specific to proj2

@cheshirekow
Copy link
Owner

Ok so I guess that since I want people installing from pypi and because pre-commit doesn't let you re-use config except in a separate repo it seems separate repo is the best choice. @mathstuf that's a good point I forgot YAML had that feature. If it works it might be a good option for someone.

In any case I created https://github.com/cheshirekow/cmake-format-precommit for pre-commit users and added a note to the README and the installation docs in this repo. I'll try to remember to keep the tags up-to-date with pypi.

Thank you @aharrison24 for taking the time to create this PR and to prototype the second repo solution and @asottile for explaining pre-commit config stuff. I'll close this PR but feel free to add additional comments if you feel there is more to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants