Update Source interface to use SourceID and JobID types #1774
Conversation
mcastorina
force-pushed
the
source-job-ids
branch
from
089a702 to
18a0523
Compare
The previous implementation used int64 for both, which can be mixed up easily. Using distinct types adds a layer of type safety checked by the compiler.
mcastorina
force-pushed
the
source-job-ids
branch
from
18a0523 to
553df09
Compare
| @@ -123,7 +123,7 @@ func (s *Source) InjectConnection(conn *sourcespb.Syslog) { | |||
| } | |||
|
|
|||
| // Init returns an initialized Syslog source. | |||
| func (s *Source) Init(_ context.Context, name string, jobId, sourceId int64, verify bool, connection *anypb.Any, concurrency int) error { | |||
| func (s *Source) Init(_ context.Context, name string, jobId sources.JobID, sourceId sources.SourceID, verify bool, connection *anypb.Any, concurrency int) error { | |||
There was a problem hiding this comment.
nit: any reason sometimes it's jobID like in line 52 and sometimes it's jobId?
There was a problem hiding this comment.
No, just ambiguity in what "camel case" means. I asked Ahrav at one point and the official Google style guide recommends jobID, so I tend to use that.
https://google.github.io/styleguide/go/decisions#initialisms
I'll make a followup PR to update the variable names, since this PR is just updating the types.
| @@ -111,7 +111,7 @@ func (c *persistableCache) shouldPersist() (bool, string) { | |||
| } | |||
|
|
|||
| // Init returns an initialized GCS source. | |||
| func (s *Source) Init(aCtx context.Context, name string, id int64, sourceID int64, verify bool, connection *anypb.Any, concurrency int) error { | |||
| func (s *Source) Init(aCtx context.Context, name string, id sources.JobID, sourceID sources.SourceID, verify bool, connection *anypb.Any, concurrency int) error { | |||
There was a problem hiding this comment.
nit: same question as jobID - looks like sometimes we use sourceID and sometimes it's sourceId. should this be standardized or do we use them for different purposes?
Yeah, by humans. Making them distinct types will allow the compiler to catch accidentally using a job ID as a source ID or vice versa. func Foo(sourceID int64, jobID int64)
func Bar(sourceID SourceID, jobID JobID)
Foo(jobID, sourceID) // no problemo (jobID and sourceID are both int64)
Bar(jobID, sourceID) // problemo (jobID and sourceID are distinct types) |
Description:
The previous implementation used int64 for both, which can be mixed up easily. Using distinct types adds a layer of type safety checked by the compiler.
Checklist:
make test-community)?make lintthis requires golangci-lint)?