chore: remove dbfake.WorkspaceBuild in favor of builder pattern #10814
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
24624dc to
70cc9f4
Compare
70cc9f4 to
c81a476
Compare
| return b | ||
| } | ||
|
|
||
| func (b BuildBuilder) Do() database.WorkspaceBuild { | ||
| func (b WorkspaceBuildBuilder) Do() database.WorkspaceBuild { |
There was a problem hiding this comment.
out of curiosity: wasn't that always Build()?
There was a problem hiding this comment.
It's usually Build() but I'm attempting to avoid a clash with WorkspaceBuild. I don't mind too much either way.
| Name: "somename", | ||
| Type: "someinstance", | ||
| Agents: []*proto.Agent{{ | ||
| Auth: &proto.Agent_InstanceId{ | ||
| InstanceId: instanceID, | ||
| }, | ||
| }}, | ||
| }) | ||
| }).Do() |
There was a problem hiding this comment.
Minor suggestion, perhaps we could rename this s/Do/Build, since a Builder Build's. I mean, the BuildBuilder is a bit unfortunate 😅, so I know, right, suggesting to add one more. But typically this naming goes Doer Do(), Builder Build(), etc.
There was a problem hiding this comment.
Yeah, I wanted to avoid Build() as it might be confusing with WorkspaceBuild.
Eventually I want the whole package to use the builder pattern and we can drop Builder from the function names. So you be like
r := dbfake.Workspace(t, db).Do()
| @@ -59,15 +59,15 @@ func WorkspaceWithAgent( | |||
| agents = m(agents) | |||
| } | |||
| ws := Workspace(t, db, seed) | |||
| WorkspaceBuild(t, db, ws, database.WorkspaceBuild{}, &sdkproto.Resource{ | |||
| NewWorkspaceBuildBuilder(t, db, ws).Resource(&sdkproto.Resource{ | |||
There was a problem hiding this comment.
Could we do NewWorkspaceBuilder instead? Then the Build function would implicitly do what we expect as well.
There was a problem hiding this comment.
I considered the same, then I wondered what we would call a "Builder" for a workspace, if we ever need one.. 😅
Edit: On second thought, perhaps you were suggesting something a bit different. Would you have one "Builder" for both workspace and workspacebuilds?
There was a problem hiding this comment.
The pattern I'm planning to follow is NewXxxxxBuilder() for the function that creates a builder (where Xxxxx is the resource type), so NewWorkspaceBuildBuilder() is the builder for a WorkspaceBuild.
I'd eventually like to convert everything in dbfake to a builder pattern, and then I think we could potentially drop the Builder suffix and the New prefix for the sake of readability. But while I'm changing things over, it's useful to be able tell what is and isn't a builder.
I'd like to convert dbfake into a builder pattern to prevent a proliferation of XXXWithYYY methods. This is one step of the way by removing the Non-builder function.