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

chore(generator): always use lib name as "generated by" #43

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

matansh
Copy link
Contributor

@matansh matansh commented Oct 17, 2024

Problem

In the project I am working on we want to use //go:generate go run github.com/globusdigital/deep-copy [args...] instead of forcing all of our contributors to install deep-copy locally. (I assume we are not the only ones)

When deep-copy is invoked via go run it creates the following comment on the auto generated files

// generated by /tmp/go-build335036942/b001/exe/deep-copy [args...]

This is problematic because:

  • Every PR out contributors will raise will contain a different go-buildxxxx which is unneeded noise
  • We run go generate ./... in our CI to validate that contributors have not forgotten to update required autogenerated code in their PRs, this CI job will always fail as it will alter the go-buildxxxx

Solution

Normalize the output of the code generator so no-op regeneration results in no diffs.

Notes

  • Even in the testing of the lib this problem was encountered, but instead of fixing it the tests were made to ignore the unwanted diff.
  • https://github.com/vektra/mockery a popular lib that generates mocks is an example of a code generator that always results in the comment // Code generated by mockery. allowing users to safely invoke it via go run .

@@ -72,7 +72,7 @@ func Test_run(t *testing.T) {
}
}

var re = regexp.MustCompile(`Code generated by .*deep-copy.*; DO NOT EDIT.`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this diff is introduced without the corresponding proposed solution the tests fail for the same reason our CI is failing

@matansh
Copy link
Contributor Author

matansh commented Oct 17, 2024

@urandom @egawata @romshark I don't know if anyone is actively subscribed to notifications from this repo, there are no reviewers automatically assigned to PRs, and I could not find contribution guidelines so I am using @ as a low tech way to ping the top contributors in order to make sure there is visibility for this proposal. (sorry not sorry 😉 )

@egawata
Copy link
Contributor

egawata commented Oct 17, 2024

@matansh
I've created PRs for this repo several times, and @urandom reviewed them. I believe he'll find this PR, so it would be good to wait a few days.

Copy link
Collaborator

@urandom urandom left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@urandom urandom merged commit dc4a8d9 into globusdigital:master Oct 22, 2024
1 check passed
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