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

feat: add pre-commit hook support #795

Closed
wants to merge 1 commit into from

Conversation

ccoVeille
Copy link
Contributor

Description

Add pre-commit.com support in mockery.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Version of Go used when building/testing:

  • 1.11
  • 1.12
  • 1.13
  • 1.14
  • 1.15
  • 1.16
  • 1.17
  • 1.18
  • 1.19
  • 1.20
  • Not relevant

How Has This Been Tested?

Locally, by following the guide I provide in installation.md file

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Comment on lines +6 to +8
additional_dependencies:
- github.com/vektra/mockery/v2@latest
entry: mockery

Choose a reason for hiding this comment

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

Rather than using additional_dependencies, the pre-commit-hook will already have access to the file system. We should just be able to use go run . to enact the mockery ... command I believe.

Also, we may only want this hook to run when .go files are in the commit. What do you think?

Suggested change
additional_dependencies:
- github.com/vektra/mockery/v2@latest
entry: mockery
entry: go run .
files: \.go$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure it will launch the code you expect to run.

I will make some tests.

But, I think keeping mockery call here would be clearer.

Choose a reason for hiding this comment

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

Using additional dependencies like this won't allow people to specify version in pre commit file because no matter what version they reference, you're pulling latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I'm unsure if there is something that can be done here, I will have to check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now,it seems to me, that it would imply to maintain the version in sync in this file vs the tag. Quite a pain

@@ -0,0 +1,6 @@
---
repos:
- repo: https://github.com/ccoVeille/mockery

Choose a reason for hiding this comment

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

Is this just in here for testing right now? Unless mockery uses itself to generate mocks, and wants to use the pre-commit functionality, I don't think this is required to get this to work for consumers.

Copy link
Contributor Author

@ccoVeille ccoVeille Jul 17, 2024

Choose a reason for hiding this comment

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

Yes, only for the test. I forgot to mention about it in the PR

I had to create a fake "2.43.3" tag on my repository to be able to make it work.


```yaml
repos:
- repo: https://github.com/ccoVeille/mockery

Choose a reason for hiding this comment

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

Suggested change
- repo: https://github.com/ccoVeille/mockery
- repo: https://github.com/vektra/mockery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept it in sync to help mockery mainteners to be able to test

But yes, I will have to change this one

```yaml
repos:
- repo: https://github.com/ccoVeille/mockery
rev: v2

Choose a reason for hiding this comment

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

nit/nb: It would be great if this could be automatically updated on new releases.

Suggested change
rev: v2
rev: v2.43.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need usually. precommit manage this well with this kind of tag

Then install the hook with the following command:

```shell
pre-commit autoupdate

Choose a reason for hiding this comment

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

Ah, this is why you have it as v2 above, calling for autoupdate will which bump the version to latest. May be better to be explicit about the versioning system, and call for pre-commit install versus autoupdate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I'm used to use autoupdate for everything.

I think "pre-commit install" would also bump it locally. I will make some tests.

Then I will update the documentation based on my tests

@@ -0,0 +1,6 @@
---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for myself

I will have to add Co-Authored-By metadata

@ccoVeille
Copy link
Contributor Author

anyway @liveFreeOrCode could you confirm it "works" as this ?

@ccoVeille ccoVeille marked this pull request as draft July 18, 2024 20:21
@ccoVeille
Copy link
Contributor Author

@liveFreeOrCode (or anyone) could you confirm ? thanks

Then I will remove the oddities present to make it works with my branch, and switch to vektra/mokery one

@ccoVeille
Copy link
Contributor Author

ccoVeille commented Jul 31, 2024

Hi @liveFreeOrCode

You commented the PR, thanks.

But you didn't reply my question. Does it work on your computer now with current code I provided.

I understand you might dislike the current implementation. I agree it could be improved. But right now,I would like to get a simple reply to the following question.

Does it work?

@liveFreeOrCode
Copy link

Hi @liveFreeOrCode

You commented the PR, thanks.

But you didn't reply my question. Does it work on your computer now with current code I provided.

I understand you might dislike the current implementation. I agree it could be improved. But right now,I would like to get a simple reply to the following question.

Does it work?

Yeah sorry I'm traveling without my PC. I can't test until next week

@ccoVeille
Copy link
Contributor Author

OK thanks for replying, we can also wait

@LandonTClipp
Copy link
Collaborator

Closing due to inactivity, feel free to resurrect it if you find the time to finish it.

@ccoVeille
Copy link
Contributor Author

Hi @LandonTClipp

Please reopen.

I was waiting for someone feedbacks.

The code is ready.

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.

Feature request: pre-commit hooks
3 participants