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

in_place = true doesn't work in pre-commit #39

Closed
jonyscathe opened this issue Jan 13, 2023 · 4 comments · Fixed by #40
Closed

in_place = true doesn't work in pre-commit #39

jonyscathe opened this issue Jan 13, 2023 · 4 comments · Fixed by #40

Comments

@jonyscathe
Copy link

jonyscathe commented Jan 13, 2023

I'm going through moving any config from .pre-commit-config.yaml into other config files wherever possible to make sure that running tools from terminal has the same result as running in the pre-commit hook.

So I changed from having:

  - repo: https://github.com/pappasam/toml-sort
    rev: v0.22.1
    hooks:
      - id: toml-sort
        args:
          - --all
          - --in-place

to:

  - repo: https://github.com/pappasam/toml-sort
    rev: v0.22.1
    hooks:
      - id: toml-sort

And adding the following to pyproject.toml

[tool.tomlsort]
all = true
in_place = true

This then works fine when running something like toml-sort pyproject.toml and that fixes the file in-place.
However, when I run the pre-commit hook it doesn't run the files in-place, it just uses the --check argument from https://github.com/pappasam/toml-sort/blob/main/.pre-commit-hooks.yaml instead as command line arguments take precedence over pyproject.toml arguments. This means that both check and in_place are True and as args.check is checked first (line 357 of cli.py) so my setting in_place = true basically has no effect.

I'm not sure of the best way to fix this.
Having a standard args of --check for the pre-commit does make sense as either --check or --in-place needs to be defined when running multiple files. And defaulting to an option that doesn't result in overwritten files is best. But it would also be good if it was possible for the in_place setting in pyproject.toml to work when running toml-sort from pre-commit.
Maybe the easiest way is to change the tool behaviour so that if both in-place and check are true then it runs in-place, rather than check.
That would just mean swapping

        if args.check:
            if original_toml != sorted_toml:
                check_failures.append(filename)
        elif args.in_place:
            write_file(filename, sorted_toml)

to:

        if args.in_place:
            write_file(filename, sorted_toml)
        elif args.check:
            if original_toml != sorted_toml:
                check_failures.append(filename)

Obviously that is a fairly large change in behaviour, though I imagine very few people would have both --check and --in-place set other than in my scenario where in_place is set in pyproject.toml and --check is only being set as the default pre-commit args.
Obviously for now I can workaround this easily enough by changing the pre-commit args to define --in-place and not --check, but I would prefer not having to set the same setting twice.

@jonyscathe
Copy link
Author

Another easy way to fix would be to make a second pre-commit hook in .pre-commit-hooks.yaml called toml-sort-fix or toml-sort-in-place or something like that with a default args of --in-place rather than check.
That way no existing behavior needs to change.

@pappasam
Copy link
Owner

Thanks for clearly laying out this issue!

I'm actually the opposite to you in that I almost always manually configure my hooks based on the locally-installed tooling. So, I don't really have a strong preference here as long as any resolution 1. doesn't negatively impact too many people, and 2. doesn't introduce too much complexity to the codebase.

Based on the above requirements, I have a slight preference for your suggestion to create additional hooks. Seems like it'll preserve backwards compatibility without introducing any unintended behavioral changes.

I'll happily consider a PR if you find time to create one!

@wwuck
Copy link
Contributor

wwuck commented Jan 16, 2023

@pappasam any chance of a new release tag on github for the new pre-commit hook? pre-commit requires the tag on github as a version reference so the new hook is inaccessible until then.

@pappasam
Copy link
Owner

@wwuck sorry, done!

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 a pull request may close this issue.

3 participants