refactor v2 client to be more idiomatic

[frrist]

Please review each commit, they are distinct changes

At a high level this PR:

• Renames the client config structures to follow the function options pattern
• Passes a context to each client API method, reasoning provided in commit message
• Requires the base URL as a parameter to the constructor, see commit message for reasoning
• Removes the context.Context from the BaseRequest structure as it's now passed for each method

[wdbaruni]

This change allows finer control over each HTTP request by accepting
a context.Context in methods like GET, PUT, POST, and DELETE, aligning
with Go's idiomatic use of context for request-scoped settings, such as
timeouts and cancellation. This approach offers greater flexibility and
is consistent with the standard library's design.

We already have finer control to set context for each request inside BaseRequest while still providing a default context at the client level. I am okay with changing this if we still think it is better

Enforce base URL as mandatory in constructor; removes the illusion
of it being an 'optional' setting when it's fundamentally required for operation

I don't think this is a valid reason. This is an anti-pattern if we keep adding required fields to the constructor and optional ones as configs. Besides, this allows us to provide different ways to set the address, including passing the address as a single field (current behaviour) or pass host and port separately.
Also localhost can be a sane default, or we can do runtime validation to check for all required fields

Just to understand the bigger picture, what is the pain the current client is causing and why are we prioritizing fixing it?

[aronchick]

This change allows finer control over each HTTP request by accepting
a context.Context in methods like GET, PUT, POST, and DELETE, aligning
with Go's idiomatic use of context for request-scoped settings, such as
timeouts and cancellation. This approach offers greater flexibility and
is consistent with the standard library's design.

We already have finer control to set context for each request inside BaseRequest while still providing a default context at the client level. I am okay with changing this if we still think it is better

Enforce base URL as mandatory in constructor; removes the illusion
of it being an 'optional' setting when it's fundamentally required for operation

I don't think this is a valid reason. This is an anti-pattern if we keep adding required fields to the constructor and optional ones as configs. Besides, this allows us to provide different ways to set the address, including passing the address as a single field (current behaviour) or pass host and port separately. Also localhost can be a sane default, or we can do runtime validation to check for all required fields

Just to understand the bigger picture, what is the pain the current client is causing and why are we prioritizing fixing it?

well, if nothing else, the client calls are broken idiomatically (e.g. many functions don't have ctx as the first parameter, which is strongly guided to - https://pkg.go.dev/context#:~:text=The%20Context%20should%20be%20the,func%20DoSomething(ctx%20context.). This solves for that.

However, I do agree that using an "Option" structure is more flexible and allows for cleaner dependency injection.

[frrist]

Just to understand the bigger picture, what is the pain the current client is causing and why are we prioritizing fixing it?

As David already alluded to, the client calls are broken idiomatically - my main source of pain. As we move from the V1 client to the V2 client I want to pay this debt down now, while it's still easy.

This is an anti-pattern if we keep adding required fields to the constructor and optional ones as configs

Providing required fields as 'options' is the anti-pattern being addressed here. Previously we passed address as an option to the client each time we constructed one, it was never optional.
This change preserves the functional options pattern and requires developers to provide required fields when instantiating a client. This design is based on the principles described in https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis - worth a read.

Also localhost can be a sane default.

localhost lacks a schema (http/https) and a port. I prefer we don't embed our current defaults in the client, as they'll probably change someday. Defaults are embedded in the config which is provided to the client when it's instantiated.

or we can do runtime validation to check for all required fields

I prefer we enforce this type of check via the compiler.

[wdbaruni]

well, if nothing else, the client calls are broken idiomatically (e.g. many functions don't have ctx as the first parameter, which is strongly guided to

Yeah that makes sense. I just wanted to clarify the current implementation does support overriding the context per call, but I understand it wasn't the golang way

However, I do agree that using an "Option" structure is more flexible and allows for cleaner dependency injection.

I like it as well

This change preserves the functional options pattern and requires developers to provide required fields when instantiating a client. This design is based on the principles described in https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis - worth a read.

Good read. Thanks for sharing. Though it doesn't say required fields should go under the constructor. Regardless of the blog post, the set of required fields can change or grow in the future, and a field might be required or optional depending on other values. We need to make a tradeoff between compile time validation and maintainability of our APIs where we don't have to break the signatures very often when features are introduced. This is a standard tradeoff we had to make even with builder pattern commonly used in java.

Still, I'll leave this decision to you whether to define the address at the constructor, and whether we want to give users the option to define the host and port separately as well. I don't feel strongly towards any, as long as we don't keep adding more fields to the constructor in the future, even required fields.

localhost lacks a schema (http/https) and a port.

localhost as in the local host. Obviously we need to define the schema and port :) To be more specific https://127.0.0.1:1234 after Simon's auth by default changes are merged

I prefer we don't embed our current defaults in the client, as they'll probably change someday. Defaults are embedded in the config which is provided to the client when it's instantiated.

Don't forget that the client is our GO SDK to access Bacalhau and that developers can embed it in their client applications. There won't be bacalhau config in that setup

I prefer we enforce this type of check via the compiler.

As I mentioned above, there will be scenarios where optional fields will be required depending on others. Runtime validation is going to be needed regardless as it provides more coverage, better error messages, and more validations, such as validating the address is not blank. Though I understand the overall value of compile time checks when possible

[wdbaruni]

As we move from the V1 client to the V2 client I want to pay this debt down now, while it's still easy.

Nice. Thanks for picking this up!

[frrist]

@wdbaruni is there anything blocking approving this change?

[aronchick]

Walid touched on something critical - we need to be validating at nearly every step. We are a platform that people will SURELY misuse