feat(commands): add resume on error for comments and emails

[joe-prosser]

Adds a --resume-on-error flag for uploading comments and emails. When the api returns an error the CLI will split a particular batch into individual requests, attempt each one individually and skip any individual request that fail

[dan-ri]

Because you're taking the input comments and just converting it to Vec, it would be better to declare these functions as taking a Vec instead of a slice.

[dan-ri]

Some of this cloning looks as if it should be unnecessary, e.g. IntoUrl is implemented for &str.

[dan-ri]

It would be nice not to have to write the main body of code for this function nested inside a match branch and a conditional. Something like:

match result {
    Ok(response) => return Ok(SplitableRequestResponse {
                response,
                num_failed: 0,
            }),
    Err(error) if !Self::should_split(&error) => return Err(error),
    _ => {},
}
...

[dan-ri]

For small structs and enums (those with size <= 8 bytes, say), Copy should also be derived, as they're cheap to copy. Then clone()ing them is unnecessary.

[dan-ri]

Just write this as a.new + b.new etc. and omit let bindings?

[joe-prosser]

I added the let binding so that we would get a compiler error if new fields were added (to remind us to make any necessary changes here)

[dan-ri]

Rather than do that, you can just put a single statement like this at the top:

// Ensure new fields are added here:
let Self { new: _, updated: _, unchanged: _ } = a;

[joe-prosser]

Thanks - just pushed a slightly different implementation

[dan-ri]

You can supply a default implementation for trait methods in the trait itself that will be used if there's no implementation for the type. This should allow you to avoid repeating these trivial implementations (you'll need a Default trait bound).

[joe-prosser]

I chose not to use the Default trait initially as it doesn't make sense to have Default for these responses unless you're using the ReducableResponse trait. Don't have strong opinions here though

[dan-ri]

In that case you can make your own version of Default called ReducibleDefault or whatever.

[joe-prosser]

Implemented with Default for the sake of simplicity :)

[dan-ri]

You can define this within splittable_request, as it's a helper private to the method.

[dan-ri]

Suggested change

	pub trait ReducableResponse {
	pub trait ReducibleResponse {

[dan-ri]

Hmm, this mutation and cloning doesn't look quite right. Conceptually we should be able to reduce the successful responses, although we need to filter out and handle the failures somehow.

[dan-ri]

This is looking much better, but we should be able to do better still as this is a standard fold pattern, if you switch back to the merge trait method:

let response = body
    .split()
    .iter()
    .filter_map(|request| {
        match self.request(&method, &url, &Some(request), &query, &retry) {
            Ok(response) => Some(response),
            Err(_) => {
                num_failed += 1;
                None
            }
        }
    })
    .fold(SuccessT::empty(), |merged, next: SuccessT| merged.merge(next));

[dan-ri]

Also, you should be able to define split as returning an impl Iterator instead of a Vec and remove the collect() at the end of the implementations.