Pass progress by value when possible, replace optional with null object

[bugadani]

Follow-up for #333 that implements my suggestions. The null object allows cleaning up some of the usage sites, and taking by value is something that I hope improves usability.

Replacing all dyn ProgressCallbacks instances is indeed not possible, but it is not a big deal.

@jessebraham please don't feel in any way pressured to accept this. This PR may or may not actually be a good idea and I only intend this as an illustration of what I had in mind. I did not think of it too deeply and I might have missed use cases where the PR might hurt more than help.

[jessebraham]

Thanks for the PR demonstrating this. I'm not going to say a hard "no" at this point, but currently I think Option is more clear about the intent, personally. I recognize there are pros and cons to each approach, so I think this ends up being a pretty subjective decision either way. I'm going to cut a new release candidate soon, so maybe we can revisit this after that.

[bugadani]

I'm fine with returning to this later, but I want to share my opinion(s) before I forget :) This is related to the optional type, not the value-vs-reference part of the PR. I understand that I might be forcing you into putting more effort into this API than necessary, so definitely feel free to completely disregard me. Still, I write the rest of this comment, just in case.

I understand that there are cases where users might want a runtime optional callback - we can impl<T: ProgressCallbacks> ProgressCallbacks for Option<T> I think to support these use cases, for example.

In my opinion, Option<impl ProgressCallbacks> as a function argument is redundant. It's easier to think about "this function needs an object (that might do nothing)" than "think function maybe needs an object".

In my opinion, the type Option<impl ProgressCallbacks> is more complex than it needs to be.

In my opinion, Option<impl ProgressCallbacks> optimizes for the negative case, i.e. not using progress callbacks. This optimization makes the (in my opinion, more important) use case of actually getting progress information a tiny bit more cumbersome.

In my opinion, if the concern is discoverability of the null implementation, we can support that by documentation. Either on the methods, or on the trait, or both.

Edit: as it turns out, finding the default EspflashProgress is equally difficult, right now as the proposed null object would be.

[jessebraham]

This is pretty outdated (and we've already bumped the major version) so closing this. Maybe something we can revisit in the future.

[bugadani]

In the big picture, it matters fairly little, though looking at the amount of comments I left here I probably felt a bit more connected to the issue in January 😅