-
Notifications
You must be signed in to change notification settings - Fork 50
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
clean up node/gendemo regeneration #123
Conversation
Not sure if want to merge this or not yet: regenerating results in a diff that undoes some of #119 ; apparently we'd have to carry some of that stuff back up into the codegenerator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not realise I had changed some of the generated files. A lot of the changes were automated, so I didn't really pay close attention to whether the files were generated.
Sounds good to me anyway. When I inevitably fix the generator, all the generated code will be fixed once again. This is a bit of diff churn, but it's small, and it's a good step anyway.
node/gendemo/gen_trigger.go
Outdated
//go:generate go run gen.go | ||
//go:generate gofmt -w . | ||
|
||
package gendemo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tip: you can use doc.go for this, which also nudges you towards writing a package godoc, which you should do anyway :) I like having doc.go files with just the package godoc and go:generate lines that don't belong elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a common gopher convention? I'd kind of like it if there was a convention pattern in filename that I can visually scan for to guess if some kind of generation is in order or not. x and x_trigger would do it for me. But I'm probably inventing that "convention", which doesn't make it much of a convention either.
I was copying what you did in another repo here with the exact invocations (edit: but not the file, because:) I have to admit, when I found the generate commands in doc.go in that other repo, I made a bit of a "Hæ?" sound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, the general convention is "put the generate line next to the code using the generated files". If that doesn't apply, then you find some package-generic file to put it in, like main.go, pkgname.go, or doc.go. I like that last one because I find it easier to know that all the package-scoped special comments will be there.
I do think that a file like gen.go is a bit unnecessary. If you don't have a doc.go because the godoc is in a pkgname.go, then put the generate lines there too. If you do have a doc.go, then two tiny Go files with just comments feels a bit silly. And if you are in neither case because you don't have a godoc... add one :)
I was copying what you did in another repo here with the exact invocations, but I have to admit, when I found the generate commands in doc.go in that other repo, I made a bit of a "Hæ?" sound.
Where did I use a trigger file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a way to summarize my thoughts is - if you can avoid an extra tiny file... avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, fine, I'll roll with it.
I suppose there's also some utility to having a single file per package, because any steps like gofmt shouldn't need to be repeated even if one were to have more than one generator at work that requires other files.
Do regeneratation. Some diff churn: the codegen doesn't yet jive smoothly with a recent change made in service to go vet. We'll want to straighten that out soon, I guess.
ada5c7d
to
7849b87
Compare
…d use package docs.
Code generation now has standard triggers (
go generate
will do the right thing) based on what @mvdan did in his hamt work (...instead of my previous weird hack using test packages, which was terrible).