-
Notifications
You must be signed in to change notification settings - Fork 113
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
Conversation
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 |
Also curious what it does when a project already has a |
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: 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: 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. |
👋 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 Happy to answer any other questions about this -- the docs for what each of the fields mean are documented here |
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. |
f1254e8
to
d5f5e43
Compare
- 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.
d5f5e43
to
6a9331b
Compare
@qibint I saw there was some recent interest in this PR, so I rebased it on to |
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 |
.. code:: yaml | ||
|
||
repos: | ||
- repo: https://github.com/cheshirekow/cmake_format.git |
There was a problem hiding this comment.
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?
@asottile can certainly provide better information, but I've looked in to this and I agree with you that 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 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 I've set up my own repo to demonstrate this at |
yes that is where it looks and that is only where it looks |
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 |
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"
contains all the needed information, without an additional unversioned remote dependency added to the project. |
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 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 thanks for reading and hope you have a good day :) |
I didn't mean to be condescending 😕. I appreciate the help.
Sorry, I must have missed it. I didn't see any disadvantages listed above... What are they?
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.
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. |
yes of course it works, but it's a worse user experience
(I'll put them in a list so it's easier to digest)
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? |
ah I made a typo above, it should be |
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:) |
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:
I don't really see a user experience difference. Regarding your list of (dis)advantages:
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.
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
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
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.
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 |
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
what I mean here is it's trivial for someone to take
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!
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
an odd choice but that's on you :) can't star / make bug reports from pypi but ok |
I just recalled another case where the For example:
vs
(etc. etc.) |
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. |
yep! the repo-based hooks are essentially
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 👍 |
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 |
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. |
to easily add a cmake-format pre-commit hook to their git project.