Always have a link to a GitHub issue for TODOS?

[bufdev]

This came up re: #863

In our codebase (including gRPC TODOS), I count 97 TODOS in our non-generated golang code, almost none of which have links to GitHub issues. Should we enforce a link to a GitHub issue? I can probably codify this in linting as well.

$ alias agg
alias agg='ag   --nogroup   --ignore '\''*\.pb\.go'\''   --ignore '\''*\.pb\.gw\.go'\''   --ignore '\''*\.pb\.yarpc\.go'\''   --ignore '\''*\.gen\.go'\''   --ignore '\''Godeps'\''   --ignore '\''vendor/'\''   --file-search-regex '\''.go'\'''
$ agg TODO
api/encoding/inbound_call.go:60:	// TODO(abg): Maybe we should copy attributes over so that changes to the
api/transport/header.go:28:	// TODO: Deal with unsupported header keys (anything that's not a valid HTTP
api/transport/request.go:69:	// TODO (#788): Include headers once we can omit PII.
api/transport/router.go:29:// TODO: Until golang/mock#4 is fixed, imports in the generated code have to
api/transport/response.go:41:	// TODO(abg): Ability to set individual headers instead?
api/transport/transporttest/reqres.go:66:// TODO: Headers like User-Agent, Content-Length, etc. make their way to the
bench_test.go:151:	headers := []byte{0x00, 0x00} // TODO: YARPC TChannel should support empty arg2
dispatcher.go:74:	// TODO(shah): Export this when we're ready to deploy a branch in
dispatcher.go:225:	// TODO (shah): add a *pally.Registry too.
dispatcher_test.go:238:			// TODO: Include the name of the outbound in the error message
dispatcher_test.go:273:			// TODO: Include the name of the outbound in the error message
encoding/raw/inbound_test.go:78:			// TODO consistent error messages between languages
encoding/thrift/inbound.go:119:// TODO(apb): reduce commonality between Handle and HandleOneway
encoding/thrift/thriftrw-plugin-yarpc/client.go:62:</* TODO(abg): Pull the default routing name from a Thrift annotation? */>
encoding/x/protobuf/outbound.go:81:	// TODO: the error from Call will be the application error, we might
encoding/x/protobuf/protoc-gen-yarpc-go/main.go:32:// TODO: there is some crazy bug with protobuf that if you declare:
internal/crossdock/client/errorshttpclient/behavior.go:108:	// TODO: Uncomment. Currently failing with Node.
internal/crossdock/client/errorstchclient/behavior.go:85:		// TODO test invalid headers
internal/crossdock/client/gauntlet/behavior.go:532:		// TODO: Once all other languages implement the gauntlet test
internal/crossdock/client/tchserver/raw.go:46:	// TODO headers should be at yarpc, not transport
internal/crossdock/client/tchclient/thrift.go:399:// TODO once other servers implement the gauntlet, this func should be removed
internal/crossdock/server/start.go:49:// TODO(abg): We should probably use defers to ensure things that started up
internal/crossdock/server/yarpc/phone.go:97:	// TODO use yarpc.Service for caller
internal/crossdock/server/yarpc/phone.go:102:		Service:   "yarpc-test", // TODO use yarpc.Service
internal/crossdock/server/yarpc/gauntlet.go:70:		// TODO raise TException once we support it. Meanwhile, return an
internal/errors/transport.go:136://TODO(apb): add tests for UnsupportedTypeError error
internal/observerware/middleware.go:51:	// TODO (shah): Add timing info to the request struct so that we don't lose
internal/pally/counter.go:48:	// TODO: Implement prometheus.Metric directly, which allows us to avoid
internal/pally/gauge.go:40:	// TODO: Implement prometheus.Metric directly, which allows us to avoid
internal/pally/registry.go:46:	// TODO: To serve our own Prometheus endpoints, we only need to implement
internal/pally/registry.go:75:	// TODO: Consider whether we should automatically scrub
internal/sync/lifecycle.go:199:	// TODO replace with DPanic log
peer/x/peerheap/list.go:88:	return pl.once.Stop(pl.clearPeers) // TODO clear peers
peer/x/roundrobin/list.go:325:		// TODO: log error
peer/x/roundrobin/list.go:331:		// TODO: log error
transport/http/header_test.go:58:// TODO(abg): Test handling of duplicate HTTP headers when
transport/http/inbound.go:96:	// TODO factor out transport and return it here.
transport/http/inbound_test.go:106:	// TODO transport lifecycle
transport/http/outbound.go:153:	// TODO factor out transport and return it here.
transport/http/outbound.go:369:	// TODO Behavior for 300-range status codes is undefined
transport/tchannel/channel_outbound.go:86:	// TODO: Should we create the connection to HostPort (if specified) here or
transport/tchannel/channel_outbound.go:129:			// TODO(abg): Set TimeoutPerAttempt in the context's retry options if
transport/tchannel/channel_outbound.go:141:			// TODO(abg): Set TimeoutPerAttempt in the context's retry options if
transport/tchannel/channel_outbound.go:157:		// TODO(abg): This will wrap IO errors while writing headers as encode
transport/tchannel/channel_outbound.go:172:		// TODO(abg): This will wrap IO errors while reading headers as decode
transport/tchannel/channel_transport.go:145:		// TODO(abg): Find a way to export this to users
transport/tchannel/channel_transport.go:148:	// TODO(abg): If addr was just the port (":4040"), we want to use
transport/tchannel/channel_outbound_test.go:375:			// TODO: If we change Start() to establish a connection to the host, this
transport/tchannel/channel_outbound_test.go:404:			// TODO: If we change Start() to establish a connection to the host, this
transport/tchannel/channel_outbound_test.go:435:			// TODO: If we change Start() to establish a connection to the host, this
transport/tchannel/handler.go:96:		// TODO: log error
transport/tchannel/handler.go:109:	// TODO: log error
transport/tchannel/handler.go:145:	defer rw.Close() // TODO(abg): log if this errors
transport/tchannel/header.go:83:	// TODO: zero-alloc version
transport/tchannel/options.go:32:// TODO update above when NewTransport is real.
transport/tchannel/outbound.go:103:		// TODO(abg): Set TimeoutPerAttempt in the context's retry options if
transport/tchannel/outbound.go:121:		// TODO(abg): This will wrap IO errors while writing headers as encode
transport/tchannel/outbound.go:136:		// TODO(abg): This will wrap IO errors while reading headers as decode
transport/tchannel/outbound_test.go:325:	// TODO: If we change Start() to establish a connection to the host, this
transport/tchannel/outbound_test.go:351:	// TODO: If we change Start() to establish a connection to the host, this
transport/tchannel/outbound_test.go:379:	// TODO: If we change Start() to establish a connection to the host, this
transport/tchannel/transport.go:72:	// TODO consider surfacing again, instead of err on start
transport/tchannel/transport.go:179:		// TODO(abg): Find a way to export this to users
transport/tchannel/transport.go:182:	// TODO(abg): If addr was just the port (":4040"), we want to use
transport/tracer_test.go:91:	// TODO: Use port 0 once https://github.com/yarpc/yarpc-go/issues/381 is
transport/tracer_test.go:158:	t.Skip("TODO this test is flaky, we need to fix")
transport/tracer_test.go:217:	t.Skip("TODO this test is flaky, we need to fix")
transport/x/grpc/codec.go:50:	// TODO: faking this as proto to be compatible with existing grpc clients
transport/x/grpc/handler.go:173:	// TODO: do we always want to return the data from responseWriter.Bytes, or return nil for the data if there is an error?
transport/x/grpc/handler.go:183:		// TODO: http propagates this on a span
transport/x/grpc/handler.go:184:		// TODO: spinning up a new goroutine for every request
transport/x/grpc/handler.go:186:		// TODO: have to use context.Background() because context is cancelled in crossdock
transport/x/grpc/inbound.go:96:		// TODO: does this actually work for yarpc
transport/x/grpc/inbound.go:104:		// TODO there should be some mechanism to block here
transport/x/grpc/inbound.go:112:		// TODO Server always returns a non-nil error but should
transport/x/grpc/inbound.go:130:	// TODO: router.Procedures() is not guaranteed to be immutable
transport/x/grpc/inbound.go:165:		// TODO: what if two procedures have the same serviceName and methodName, but a different service?
transport/x/grpc/inbound.go:166:		// TODO: should we handle procedure.Encoding somehow?
transport/x/grpc/response_writer.go:46:	// TODO: handle error
transport/x/grpc/outbound.go:123:	// TODO: use pooled buffers
transport/x/grpc/outbound.go:150:	// TODO: redial
transport/x/grpc/outbound.go:155:		// TODO: does this actually work for yarpc
transport/x/grpc/util.go:34:	// TODO: this is hacky
transport/x/grpc/util.go:42:	// TODO: do we really need to do url.QueryEscape?
transport/x/grpc/util.go:64:		// TODO: this is hacky
transport/x/grpc/util.go:67:	// TODO: this is hacky
transport/x/redis/outbound.go:63:	// TODO
transport/x/redis/inbound.go:82:	// TODO
transport/x/redis/inbound.go:138:			// TODO: log error
transport/x/redis/inbound.go:160:	// TODO: logging
x/config/configurator.go:78:	// TODO: Panic if a transport with the given name is already registered?
x/config/configurator.go:233:		// TODO: Maybe we should keep track of the inbound name so that if
x/config/spec.go:137:	// TODO(abg): Make error returns optional -- if the function doesn't
x/config/spec.go:171:	// TODO(abg): Allow functions to return and accept specific
x/config/spec.go:404:		// TODO: We can make this smarter by making transport.Transport
x/config/spec.go:556:	// TODO: maybe rename to buildable?
x/config/spec.go:624:	// TODO(abg): Do we want to support top-level map types for configuration

[willhug]

Speaking from experience, finding a complicated TODO with no followup has been a frustrating experience (especially when trying to learn a new codebase).

In my opinion, it should be common practice to link to a task to follow up on any TODOs we write. When someone happens upon code with a TODO without an issue, it requires more inference and sleuthing to determine what the next steps should be. It also allows us to link issues in the codebase with issues in our tracking systems.

We should retroactively link all those TODOs to tasks so they don't get lost in the shuffle and we can get people looking at them and decide whether they are worth it or not (And if we determine they are not worth it, it would be cool if we could link the TODO comments to the tasks to strategically delete them).

[bufdev]

Definitely agree :)

…

On Tue, Apr 11, 2017 at 10:16 PM Will Hughes ***@***.***> wrote: Speaking from experience, finding a complicated TODO with no followup has been a frustrating experience (especially when trying to learn a new codebase). In my opinion, it should be common practice to link to a task to follow up on any TODOs we write. When someone happens upon code with a TODO *without an issue*, it requires more inference and sleuthing to determine what the next steps should be. It also allows us to link issues in the codebase with issues in our tracking systems. We should retroactively link all those TODOs to tasks so they don't get lost in the shuffle and we can get people looking at them and decide whether they are worth it or not (And if we determine they are not worth it, it would be cool if we could link the TODO comments to the tasks to strategically delete them). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#919 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECGvIgEHgiNPdNhy-Oxph7A1RS-1Xk0ks5ru9-0gaJpZM4M6DGa> .