yarpc / yarpc-go

A message passing platform for Go
MIT License
401 stars 101 forks source link

Stop using go generate? #627

Closed bufdev closed 7 years ago

bufdev commented 7 years ago

Way back in the day when the original blog post came out, I loved the idea and started using go generate a bunch, but then as I got older I started to dislike shell commands being declared in various files, and having failures when programs weren't installed. I moved everything to my Makefiles, which was the SOT for everything else development related, and it made sense that generation should go there too. It made my life a lot easier to know what was really happening just by looking at one file, and knowing the invocation order without having to think about package dependencies.

Would this be something that could potentially be done here? I basically am advocating for moving all these to a generate target:

api/peer/peer.go:23://go:generate mockgen -destination=peertest/peer.go -package=peertest go.uber.org/yarpc/api/peer Identifier,Peer
api/peer/list.go:29://go:generate mockgen -destination=peertest/list.go -package=peertest go.uber.org/yarpc/api/peer Chooser,List
api/peer/transport.go:23://go:generate mockgen -destination=peertest/transport.go -package=peertest go.uber.org/yarpc/api/peer Transport,Subscriber
api/transport/clientconfig.go:23://go:generate mockgen -destination=transporttest/clientconfig.go -package=transporttest go.uber.org/yarpc/api/transport ClientConfig,ClientConfigProvider
api/transport/handler.go:33://go:generate mockgen -destination=transporttest/handler.go -package=transporttest go.uber.org/yarpc/api/transport UnaryHandler,OnewayHandler
api/transport/handler.go:38://go:generate stringer -type=Type
api/transport/outbound.go:25://go:generate mockgen -destination=transporttest/outbound.go -package=transporttest go.uber.org/yarpc/api/transport UnaryOutbound,OnewayOutbound
api/transport/inbound.go:23://go:generate mockgen -destination=transporttest/inbound.go -package=transporttest go.uber.org/yarpc/api/transport Inbound
api/transport/router.go:28://go:generate mockgen -destination=transporttest/router.go -package=transporttest go.uber.org/yarpc/api/transport Router,RouteTable
encoding/thrift/inbound_test.go:39://go:generate mockgen -source=register.go -destination=mock_handler_test.go -package=thrift
encoding/thrift/inbound_test.go:40://go:generate mockgen -destination=mock_protocol_test.go -package=thrift go.uber.org/thriftrw/protocol Protocol
encoding/thrift/gen.go:23://go:generate thriftrw internal.thrift
encoding/thrift/gen.go:24://go:generate ../../scripts/updateLicenses.sh
internal/crossdock/thrift/gen.go:23://go:generate thriftrw --plugin=yarpc echo.thrift
internal/crossdock/thrift/gen.go:24://go:generate thriftrw --plugin=yarpc oneway.thrift
internal/crossdock/thrift/gen.go:25://go:generate thriftrw --plugin=yarpc gauntlet.thrift
internal/crossdock/thrift/gen.go:27://go:generate thrift-gen --generateThrift --inputFile echo.thrift
internal/crossdock/thrift/gen.go:28://go:generate touch gen-go/echo/.nocover
internal/crossdock/thrift/gen.go:30://go:generate thrift --gen go:thrift_import=github.com/apache/thrift/lib/go/thrift gauntlet_apache.thrift
internal/crossdock/thrift/gen.go:31://go:generate touch gen-go/gauntlet_apache/.nocover
internal/crossdock/thrift/gen.go:33://go:generate thrift-gen --generateThrift --inputFile gauntlet_tchannel.thrift
internal/crossdock/thrift/gen.go:34://go:generate touch gen-go/gauntlet_tchannel/.nocover
internal/crossdock/thrift/gen.go:38://go:generate ../../../scripts/updateLicenses.sh
internal/examples/oneway/gen.go:23://go:generate thriftrw --plugin=yarpc sink.thrift
internal/examples/oneway/gen.go:24://go:generate ../../../scripts/updateLicenses.sh
internal/examples/oneway/main.go:37://go:generate thriftrw --plugin=yarpc sink.thrift
internal/examples/protobuf-keyvalue/kv/gen.go:23://go:generate ../../../../scripts/protoc-go.bash kv.proto
internal/examples/protobuf-keyvalue/kv/gen.go:24://go:generate ../../../../scripts/protoc-yarpc-go.bash kv.proto
internal/examples/thrift-hello/hello/gen.go:23://go:generate thriftrw --plugin=yarpc echo.thrift
internal/examples/thrift-hello/hello/gen.go:24://go:generate ../../../../scripts/updateLicenses.sh
internal/examples/thrift-hello/hello/main.go:37://go:generate thriftrw --plugin=yarpc echo.thrift
internal/examples/thrift-keyvalue/keyvalue/gen.go:23://go:generate thriftrw --plugin=yarpc kv.thrift
internal/examples/thrift-keyvalue/keyvalue/gen.go:24://go:generate ../../../../scripts/updateLicenses.sh
serialize/gen.go:23://go:generate thriftrw internal.thrift
serialize/gen.go:24://go:generate ../scripts/updateLicenses.sh
transport/x/redis/client.go:25://go:generate mockgen -destination=redistest/client.go -package=redistest go.uber.org/yarpc/transport/x/redis Client
bufdev commented 7 years ago

Also for example, scripts/updateLicenses.sh is called like 4 times, since I'm assuming this was done any time a new dependency graph came up (and it's not working right now properly) :)

HelloGrayson commented 7 years ago

@peter-edge that sounds reasonable 👍

bufdev commented 7 years ago

@breerly We might want to get this in then sooner than later https://github.com/yarpc/yarpc-go/pull/632 I just did a merge from upstream and there was already another //go:generate added, the branch passes make test etc, and I verified 'make generate' does nothing differently than the existing make generate.

HelloGrayson commented 7 years ago

@abhinav seemed to have some reservations - can you elaborate on those here?