replicate / replicate-go

Go client for Replicate
https://replicate.com
Apache License 2.0
65 stars 9 forks source link

Update `NewClient` to use functional options pattern #3

Closed mattt closed 1 year ago

nickstenning commented 1 year ago

Just a couple of thoughts as you seem to be on a roll with this:

mattt commented 1 year ago

Thanks for all of your feedback so far, @nickstenning 😄

  • it's a breaking change, but I'd be tempted to get rid of auth string too, replacing it with a default strategy of fetching the token from the OS env, and allowing it to be overridden with a WithToken functional option.

This is an interesting one. The Python client has this automatic behavior (whereas the JavaScript, Swift, and Elixir clients don't), and this has caused some users to open a few issues asking about it in the Python client.

My subjective take is that I appreciate explicitness more than any convenience gained by the line of code that saves. But then again, I have a much more limited understanding of what's expected among Go devs to make that call with any certainty.

  • make the fields on client private

Again, I'll defer to you or anyone else with a better sense of what's generally expected. I'd be interested to hear the case for one way over another.

nickstenning commented 1 year ago

Ah, right. I'm agnostic on the question of whether to autoconfigure based on the environment variable. Explicit is good, and I'd be totally fine with a signature of

func NewClient(options ...Option) *Client
// or
func NewClient(options ...Option) (*Client, error)

and then set a token with

client := replicate.NewClient(replicate.WithToken(token))

Passing the token in as a string is /fine/ but I think a single abstraction for setting config is better, and one that preserves encapsulation better still, because it allows us to do stuff like

NewClient(WithOptionsFromEnv())
NewClient(WithOptionsFromFile(path))

etc.

That's also why I think the fields on Client need to be private. If we make them private then we can easily change the internal implementation to store them on a different struct than Client itself if we find we need that in future.