feat: Refactor ui package to make it testable

[Happycoil]

Introduces a UI type to enable testing. It keeps the default behaviour of writing to Stdin/Stdout, but allows for configuring any io.Reader and io.Writer. The functions of the ui package are now methods, and all consumers of the package have been updated to use the type.

[emilkje]

Hello @Happycoil,

Thank you for your contribution!

Your updates are well-timed and align seamlessly with our recent overhaul of the internal packages. Unfortunately, this has led to some merge conflicts with your pull request, for which I apologize.

Regarding the proposed factory signature that employs a functional pattern for UI configuration, such as ui.WithRead(os.StdIn), I have some reservations. While I recognize and value the flexibility it introduces, it presents potential challenges for serialization and deserialization of options. This complexity is manageable within the CLI context, where we have direct oversight of the consumer's implementation. However, our latest refactoring efforts are directed towards enhancing the project's usability and facilitating the integration of its decoupled components into other Go projects. It is in these scenarios that the proposed approach might introduce complications.

Could we explore alternative strategies that maintain ease of use for external consumers while preserving the intended flexibility? Your thoughts on this would be greatly appreciated.

[Happycoil]

Hi @emilkje,

I don't understand the objection. What serialization and deserialization of options?

The functional option pattern is a well-known pattern. It's a pattern which allows us to write a NewThing function which can be opinionated about the struct's values, while also allowing overwriting zero, one or more of them by passing these WithSomeOtherThing functions which you'll see in many other Go libraries. It is particularly useful for making things like this testable. Most structs are simple and don't require them, but this is a case where they're appropriate.

There are other ways of doing that-ish, but they usually mean that you have to pass the parameters every time you want to use the library, either as arguments to the function (i.e. AskYesNo("foo", true, os.Stdout)), in the struct itself every time you want to use the library (and beware the nil pointers if you forget), or in some config struct.

While it's a bit difficult to see the point the first time you see the pattern, it's not a needless complication. It gives you better control of how to call your API while increasing its flexibility for testing purposes. It is also very helpful if your library has many parameters, but you usually don't need to pass more than a couple of them.

In this case it's appropriate because:

• We need to do dependency injection to test this
• In 99.99% of other use cases, we want to use os.Stdin/os.Stdout

So almost every other consumer out there doesn't want the io.Reader and io.Writer to be anything else than os.Stdin and os.Stdout, and by writing it this way the consumers can just call ui.NewUI() and get that. The tests on the other hand can modify the reader and writer to make assertions on the results. It's exactly the right thing to do for the stated purpose, which is to make the library decoupled and consumable by other code.

"Alternative strategies", would be to just skip the options pattern and require passing the reader and writer every time. As a consumer of the library I would be annoyed.

Here is how the API for the library looks, and how it would look with just plain old struct members:

// With the functional pattern
ui := ui.NewUI()
ui.AskYesNo(query, true)

// Without the functional pattern
// This could also be in a config struct, but it's so obvious you should never
// be forced to pass this
ui := ui.UI{
	Reader: os.Stdin,
	Writer: os.Stdout,
}
ui.AskYesNo(query, true)

// Without the functional pattern, forgetting to set struct members
// I wouldn't expect to have to set
ui := ui.UI{}
ui.AskYesNo(query, true) // nil pointer dereference

// Or just use functions with tons of mystery parameters
// AKA how Microsoft writes their Go libraries...
ui.AskYesNo(query, true, os.Stdin, os.Stdout)

[emilkje]

Thanks for the detailed clarification @Happycoil

I'm familiar with the pattern. My hesitation stems from past experiences as a consumer, where I found it particularly challenging to save configurations to a disk or database when dealing with functional options. In hindsight I admit that the serialization/deserialization point is unfounded as we're not dealing with primitive types in this case. It will have implications on the cohesity of the overall API surface though.

I appreciate your input and am looking forward to incorporating this into the project. Let's proceed with the merge once everything is in order!

[emilkje]

LGTM

[emilkje]

Let me know if you want to address the merge conflicts or I will take care of it soon.