Skip to content
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

refactor: Extract store package #100

Merged
merged 10 commits into from
Oct 16, 2024
Merged

refactor: Extract store package #100

merged 10 commits into from
Oct 16, 2024

Conversation

jdkaplan
Copy link
Collaborator

Now that we've cleaned up database.go a bit, it looks like a good candidate for extraction into its own package. This makes it easier to test and explain to new contributors.

I named the package store because firestore would have collided with the actual Firestore client package and fs would have collided with the actual standard library filesystem package. We can re-refactor into a better name if we find one!

Some extra changes I made along the way:

  • Rename FirestoreFooDB to store.FooClient
  • Scope SecretsClient to its one collection
  • Extract a pbtest package now that the test Firestore client is used in two test packages.
  • Use our new assert.Equal helper instead of defining .Equal methods on DB structs.

@jdkaplan jdkaplan requested a review from cceckman October 14, 2024 20:17
Copy link
Collaborator

@cceckman cceckman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I have some style thoughts for a future cleanup:

  • Early-bind the firestoreClient, so we don't have to deal with pl.db in every call
  • Some places with arguments recurser.ID, recurser -- seems like a redundant argument?

Maybe I'll just send the PR :)

@cceckman
Copy link
Collaborator

* Early-bind the `firestoreClient`, so we don't have to deal with `pl.db` in every call

Er, I guess this is what we're getting away from: having four different objects in the top-level. Hm.

@cceckman cceckman merged commit 235495c into main Oct 16, 2024
3 checks passed
@cceckman cceckman deleted the jdkaplan-extract-store branch October 16, 2024 13:32
@jdkaplan
Copy link
Collaborator Author

  • Early-bind the firestoreClient, so we don't have to deal with pl.db in every call

Er, I guess this is what we're getting away from: having four different objects in the top-level. Hm.

Yeah, I go back-and-forth on this one a lot.

My motivation for doing it in this PR was to make the "constructor" for PairingLogic easier, because I'm working toward a dedicated pairing package to host it.

I don't mind the struct itself having all four of them early-bound, though! I just don't like main knowing that. Once it has a constructor, I wouldn't mind if we undo the late-binding change.

  • Some places with arguments recurser.ID, recurser -- seems like a redundant argument?

Agreed! There's even one place I know that the first arg is (now) even totally unused:

func (r *RecursersClient) Set(ctx context.Context, _ int64, recurser *Recurser) error {

Maybe I'll just send the PR :)

Yes, please! Or if not, we can offer it as a "good first issue" for someone who wants to get started learning Go. Either way makes me happy! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants