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

Binding pre-commit-config.yaml package versions to repo package.json? #945

Closed
kortina opened this issue Feb 14, 2019 · 25 comments
Closed

Binding pre-commit-config.yaml package versions to repo package.json? #945

kortina opened this issue Feb 14, 2019 · 25 comments

Comments

@kortina
Copy link

kortina commented Feb 14, 2019

I was wondering if there is a recommendation for binding the version numbers specified in .pre-commit-config.yaml to versions in my repo's package.json?

We already maintain, for example, version numbers for prettier and eslint in a package.json and I am worried our pre-commit config will get out of sync w these.

Any way to reference a package file from the pre-commit config to try to avoid this?

@asottile
Copy link
Member

there is not a way to reference / include outside of the .pre-commit-config.yaml -- for instance see #730 / #880

that said, the desire to implement this has come up a few times and the suggestion I've given is to write a hook to synchronize the two. I haven't seen an implementation of that yet but would welcome one if it were implemented.

That way you could do something like:

-   repo: https://github.com/pre-commit/sync-versions
    rev: v1.0.0
    hooks:
    -   id: sync-versions

by default it would probably use

    files: ^(\.pre-commit-config.yaml|package-lock\.json|requirements.*\.txt)$

or something of the sort and then expand to support more languages in the future?

@kortina
Copy link
Author

kortina commented Feb 14, 2019

OK, that was going to be my backup plan.

And in this case, https://github.com/pre-commit/sync-versions is a fictitious repo yet to be written, correct?

@asottile
Copy link
Member

indeedy

@peterwj
Copy link

peterwj commented Aug 27, 2019

I'm running into this same issue, too, in a Javascript project. Right now, I have our linting dependencies specified twice (once in package.json, once in .pre-commit-config.yml), and it's challenging to keep the two in sync, especially as I use an automated service for upgrading dependencies in package.json. I'd like to centralize dependencies in one place to reduce the odds of dependency drift.

Ideally, I'd like to keep the dependencies in package.json, as that's the standard place to put Javascript dependencies. We get to benefit from automated dependency upgrade services, and our CI doesn't need to install Python and pre-commit in order to run linters.

Is there a way to configure pre-commit to simply run a shell command in my repository, so that I can implicitly use package.json's dependencies and remove the duplicate specification?

@asottile
Copy link
Member

language: system

@asottile
Copy link
Member

In particular, this bit of the docs: https://pre-commit.com/#repository-local-hooks

@peterwj
Copy link

peterwj commented Aug 28, 2019

@asottile -- thank you very much for both the pointer and the incredibly fast response!

@gtback
Copy link

gtback commented Aug 30, 2019

I just ran into this same issue today. @peterwj are you already working on a hook? If not, I can try to take a look at making one pretty soon.

@peterwj
Copy link

peterwj commented Sep 2, 2019

@gtback I ended up using the language: system and repository local hooks approach that Anthony linked above -- the entry calls an npm script that runs eslint over the files passed in via command line args. works great, since the only place I specify dependencies now is package.json.

@gtback
Copy link

gtback commented Sep 3, 2019

@peterwj any chance you could share that script, for example in a Gist, unless it's closely tied to your code itself? I'm sure others (selfishly, including myself) could benefit from it, even though it's probably a pretty simple script.

@peterwj
Copy link

peterwj commented Sep 3, 2019

sure!

in package.json:

...
    "scripts": {
        "eslint:fix": "eslint --fix"
    },
...

in .pre-commit-config.yaml:

repos:
  - repo: local
    hooks:
      - id: eslint
        name: eslint
        language: system
        files: '\.(js|vue)$'  # replace this with a regex for files you want to lint 
        entry: npm run eslint:fix

@gtback
Copy link

gtback commented Sep 3, 2019

Thanks!

@bhrutledge
Copy link

Out of curiosity, I went down the rabbit hole of writing a script to sync hooks installed by pre-commit with my local development environment. It's currently part of an open PR at bhrutledge/trello2md#8 (comments welcome). It's limited to Python hooks, and requires pre-commit to be installed in your Python virtual environment.

I was thinking about filing a feature request issue to add this functionality to pre-commit. However, while it was fun to write, this solution feels too complex. So, I'm sharing it here for anyone who might find it interesting.

@bhrutledge
Copy link

@asottile I got an email with what seemed to be a comment from you, but I don't see it here. To clarify, the script doesn't alter the pre-commit cache (to my knowledge). Instead, it pip installs from the cached repos into my active virtual environment.

I acknowledge that this is exotic yak-shaking, and that it's relying on implementation details of pre-commit. That said, I went down that path out of curiosity about the internals of pre-commit, and because I'm apparently hung up on combining the nice features of my pre-commit with the nice features of my editor.

Happy to continue this conversation elsewhere to avoid noise, or just drop it altogether. 😉

@asottile
Copy link
Member

I misunderstood the script as running pip install --upgrade inside the pre-commit envs and after a second read noticed it was a different pip and deleted the comment, it's still using private implementation detail so I would suggest you don't do that. the "public"ish interfaces are in pre_commit.clientlib only

@Skylion007
Copy link

What exactly would the binding look like in the YAML? A special commend similar to "frozen," Seems like a lot of the tooling from autoupdate should be exposed to the public API like the write_config function.

@asottile
Copy link
Member

asottile commented May 9, 2021

so, the core problem, or at least the hardest to solve part of this, is yaml is not machine writable (I have tried ruamel.yaml which promises this feature, but it falls short in many many ways). the autoupdate machinery cheats a bit on this and uses some clever regexes to match out the versions. some mechanism to read and write additional_dependencies would necessarily be dependent on machinery like that and I don't really know how it would work

@Skylion007
Copy link

Skylion007 commented May 12, 2021

some mechanism to read and write additional_dependencies would necessarily be dependent on machinery like that and I don't really know how it would work

Why isn't this accomplishable with some clever REGEX that only parses that individual additional_dependency key? That single key should also be a valid YAML file. Maybe we could find a way just to roundtrip that one individual key and replace it? We can't guarantee a perfect roundtrip for that key, but we are writing / generating it anyway so that shouldn't be an issue. https://github.com/wwkimball/yamlpath seems like a tool designed for this purpose, but it's based on ruamel.

@asottile Do you have any examples of how it falls short? I think with the right options, ruamel seems like the only viable option (aside from some VERY complex regex to parse the additional dependency key and ONLY that key).

Only other parser I could find which might be a good match would be yq: https://github.com/mikefarah/yq but I don't know it even supports as much extraneous data and whitespace as RUAMEL would.

Alternatively, couldn't we just throw an error if the ruamel roundtrip dump fails to reproduce the original YAML file exactly before we edit anything? That seems like a good compromise. This is what the black formatter does (ensuring it can roundtrip the python before doing anything else to it).

@asottile
Copy link
Member

I'd really rather not have a partial solution, especially involving ruamel -- the author seems to have gone rogue and does not accept issues or PRs any more (and has moved to sourceforge of all places?).

concretely, a few cases it does not handle last I tried: inline comments, list formats, trailing commas, map indents

@ssbarnea
Copy link

@asottile It seems you came to same conclusion as me. That is why I forked the project more than an year ago, creating pycontribs/ruyaml#1 -- Sadly I am not sure how well maintained is that fork. All I can say is that I am happy to add new maintainers to it (already has 3).

@pokey
Copy link

pokey commented Apr 8, 2023

I've tried to get various flavors of #945 (comment) to work for me on pre-commit.ci, but no luck. I can't seem to get it to install dependencies from my package.json. Any advice?

@pokey
Copy link

pokey commented Apr 8, 2023

Fwiw I got syncing to work using meta-updater. See cursorless-dev/cursorless#1400. The setup from that PR will only work on a pnpm monorepo

@pokey
Copy link

pokey commented Apr 9, 2023

And I managed to get just running the version from package.json working using a GitHub action instead of pre-commit.ci: cursorless-dev/cursorless#1404

@WilliamDEdwards
Copy link

I'd just like to point out that - while I understand the technical considerations to not implement this feature - the lack of it is a huge PITA.

@asottile's stance seems to be that dependencies in .pre-commit-config.yaml should not be duplicated (see pre-commit/pre-commit-hooks#311 (comment), for example), but that's not viable when using IDE integrations.

@asottile
Copy link
Member

@pre-commit pre-commit locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

9 participants