Skip to content

fix: local overcommit file merges with existing .overcommit.yml file #815

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

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

julesros
Copy link
Contributor

The .local-overcommit.yml did not work as intended. When a repo level .overcommit.yml and .local-overcommit.yml both existed, only the latter applied. Further, the tests only checked that each file was loaded, not that the configs included the expected overrides.

This PR refactors:

  • Specs:
    • When .overcommit.yml exists, applies settings and has default settings still
    • When both .local-overcommit.yml and .overcommit.yml exists has default settings and both files settings applied
  • Logic to merge the .overcommit.yml file to default settings and if it exists, .local-overcommit.yml file to both

@julesros julesros force-pushed the jh/fix-local-overcommit-file branch from c97ec56 to be113dd Compare September 24, 2023 12:17
@julesros
Copy link
Contributor Author

@sds Greetings! I was using the feature I added earlier this year and realized it had a bug. Providing a fix here for your consideration.

@simoleone
Copy link

I just hit the same bug and found it rather confusing. Anything I can do to help get this merged?

@julesros
Copy link
Contributor Author

julesros commented Nov 21, 2023

I just hit the same bug and found it rather confusing. Anything I can do to help get this merged?

👋🏻 @simoleone , I need to take a look at where I left this. If it works as I had intended we can tag the maintainers for review I think. I'll try to get to it this weekend! If you'd like to test this branch locally as well and make sure it works as expected for the issue you found as well, that would probably be great.

@simoleone
Copy link

hey! Yup this works the way I think it should 😄

@simoleone
Copy link

Hey! Any way I can help out with this? 😄 . I know how free time goes with my own projects (or doesn't 🙃 ).

@julesros
Copy link
Contributor Author

Hey! Any way I can help out with this? 😄 . I know how free time goes with my own projects (or doesn't 🙃 ).

Hi! Ah so sorry, yes, time is hard to find 🙃
If you've pulled down this branch and it looks like it works, then I think we can just go ahead and ask for a review from maintainers and request it to be merged in. I had wanted to clean it up more but if it works we can go with it.

@sds sds merged commit 02f7b3e into sds:master Dec 13, 2023
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.

3 participants