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

tools: structs method generation #10817

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

tools: structs method generation #10817

wants to merge 2 commits into from

Conversation

tgross
Copy link
Member

@tgross tgross commented Jun 25, 2021

Developers are required to implement methods like Copy, Equals, Diff,
and Merge for many of the types in the structs package, and this is both
time-consuming and error prone, having resulted in several correctness bugs
from failing to copy objects retrieved from go-memdb or failing to compare
objects during plans.

This changeset introduces a prototype tool to use with go:generate
directives that can generate methods automatically for our developers. Future
changesets will include documentation for developers and porting the existing
methods to use this tool. We'll continue to debug and refine the tool as we
go.

(Co-authored with @DerekStrickland )

schmichael
schmichael previously approved these changes Jun 25, 2021
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Very excited about this, but the parsing code is daunting. More code comments might help, but I think my only ask before merging is a README describing what this is and how to use.

Comment on lines 21 to 31
//go:embed structs.copy.tmpl
var copyTmpl embed.FS

//go:embed structs.equals.tmpl
var equalsTmpl embed.FS

//go:embed structs.diff.tmpl
var diffTmpl embed.FS

//go:embed structs.merge.tmpl
var mergeTmpl embed.FS
Copy link
Member

Choose a reason for hiding this comment

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

Super excited to start using builtin embed!

@@ -0,0 +1,82 @@
package structs
Copy link
Member

Choose a reason for hiding this comment

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

To make Go happy I think we should emit a comment matching:

^// Code generated .* DO NOT EDIT\.$

https://golang.org/pkg/cmd/go/internal/generate/

@DerekStrickland
Copy link
Contributor

@schmichael @tgross I'll take a stab at a README, similar to what I am doing in this jobspec update PR #10819.

Also, I had some readability/maintainability refactorings in another branch that didn't land in time for Friday. I'll take a stab at rebasing it off of this + adding comments and then see if we want to merge.

@DerekStrickland
Copy link
Contributor

@tgross @schmichael Here's the other branch. Let me know if you'd prefer a PR targeting this one. https://github.com/hashicorp/nomad/tree/generate-structs-refactor

@tgross
Copy link
Member Author

tgross commented Jun 28, 2021

Let me know if you'd prefer a PR targeting this one.

You can probably just drop commits onto this PR if you want, but I'd strongly recommend splitting the file rename refactor from any other refactors so that folks can review them separately. Ex. right now generate-structs...generate-structs-refactor just show the whole file getting replaced.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
tgross and others added 2 commits June 26, 2024 09:27
Developers are required to implement methods like `Copy`, `Equals`, `Diff`,
and `Merge` for many of the types in the `structs` package, and this is both
time-consuming and error prone, having resulted in several correctness bugs
from failing to copy objects retrieved from go-memdb or failing to compare
objects during plans.

This changeset introduces a prototype tool to use with `go:generate`
directives that can generate methods automatically for our developers. Future
changesets will include documentation for developers and porting the existing
methods to use this tool. We'll continue to debug and refine the tool as we
go.

Co-authored-by: Derek Strickland <[email protected]>
Co-authored-by: Tim Gross <[email protected]>
* use docstring comments rather than long command lines to signal the top-level
  target types and methods
* break up into separate analyzer and generator types for clarity and reduced
  state sharing
* make only 2 passes thru AST
@tgross tgross force-pushed the generate-structs branch from e1027ea to 84a9fc6 Compare June 26, 2024 13:34
@tgross tgross marked this pull request as draft June 26, 2024 16:23
@tgross tgross dismissed schmichael’s stale review June 26, 2024 16:24

re-architected

@tgross tgross removed the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants