refactor/transform: introduce transform and improve error handling

[drahnr]

Note that this PR still lacks test adjustments required and transform is not covered yet.

Changes:

• improve the error type
• add fs-err for better errors without much chore
• introduce fn transform for transformations before artifacts hit the cache_dir (this is not quite correct now, tbd)
• introduce a type alias type Result<T> to avoid boilerplate
• use BufReader and BufWriter instead of intermediate Vec where possible
• migrate a few elements to more idiomatic expressions

[bminixhofer]

Thanks for the PR!

I also think a transform_fn field on the builder is the best solution, I planned to do it the same way. I'm not sure a transform_path_fn is necessary since the transformed data might not actually exist in file form (e. g. if no cache dir is set). That said, not having a transform_path_fn would of course mean that the file extension of the cached data is slightly incorrect. I'm fine with both.

Also thanks for fixing #33 and the expects right away too! This PR is quite subsantial so feel free to add yourself to the authors of nlprule-build. I'll have a closer look once it is finished.

[drahnr]

It seems crate brotli is f*cked. Neither smush nor brotli directly succeed, currently the relevant code is comments are prefixed with XXX. Filed dropbox/rust-brotli#52

[drahnr]

@bminixhofer a first round of review would be appreciated 🦺 it could be simplified in a few areas, but that's smth for another day.

[bminixhofer]

a first round of review would be appreciated

Sure! But likely tomorrow 🙂

It seems crate brotli is f*cked.

IMO it's fine to just use something else in the tests here. I just tried this and have the same issue (hangs indefinitely). But I don't think it is needed for this PR. It's good to have two different compression algorithms in the tests but I see no reason to use brotli specifically.

[bminixhofer]

Ok, review done 🙂 Mostly just comments and small changes. The overall approach is good!

The most significant thing I would've done differently is not using traits for the transform arguments, that adds quite a bit of boilerplate. I think we can also simplify the signature a bit.

[bminixhofer]

What's the rationale for having a trait and the generic I and Cursor<&'r mut Vec<u8>> here instead of e. g. type TransformDataFn = Box<dyn Fn(Vec<u8>) -> Result<Vec<u8>>>;? That would simplify things quite a bit and Fn(Vec<u8>) -> Result<Vec<u8>> is still expressive enough. I would argue readability is more important than performance here because the closure is likely called very infrequently.

[drahnr]

That's what I meant in my previous comment in the PR (not in the review pane). That's complexity to be removed.

I don't think it makes sense to use type aliases. Passing the type would then require the user to Box or have the trait bounds there (and hence twice).

While in this case one could argue, that a simplified fn signature might be good, I find that a bit of an anti-pattern. impl Read and impl Write can easily be "chained"/combined without intermediate buffers.

[drahnr]

I just attempted it, but having an dyn Fn(dyn Read, dyn Write) -> result::Result<(), OtherError> {} opens a can of worms, so I am not sure that's in scope of this PR.

[bminixhofer]

You're right, I agree on the impl Read / impl Write now, it is also consistent with the signature of postprocess.

Passing the type would then require the user to Box or have the trait bounds there (and hence twice).

I don't see an issue with having the bounds twice (once in the type alias, once in the transform function). Something like this would work:

pub type TransformDataFn =
    Box<dyn Fn(Cursor<Vec<u8>>, Cursor<&mut Vec<u8>>) -> result::Result<(), OtherError>>;

and

pub fn transform<F, C>(mut self, proc_fn: C, path_fn: F) -> Self
where
    C: Fn(Cursor<Vec<u8>>, Cursor<&mut Vec<u8>>) -> result::Result<(), OtherError> + 'static,
    F: TransformPathFn + 'static,
{
    self.transform_data_fn = Some(Box::new(proc_fn));
    self.transform_path_fn = Some(Box::new(path_fn));
    self
}

or am I missing something? The repetition is not perfect but changing one or the other immediately gives a compile time error so that's not an issue.

[drahnr]

See the last commit, one needs late bound life times and a bunch of boxes to be able to use dyn Read and dyn Write in the args which is needed for the get_build_dir case.

[drahnr]

Note that this now includes a bunch of lifetime trickery, which I am happy to explain.

[drahnr]

Oh, and please squash merge this 🗜️

[bminixhofer]

I think we can simplify the signature of TransformDataFn to use concrete structs. That would also remove the "lifetime trickery". 🙂

Besides that only small changes. Please also run rustfmt on the files.

[bminixhofer]

CI failed because Backblaze bandwith got exceeded, I fixed that now. But there's a compilation error now.

[drahnr]

Should be fixed now.

Sidenote: Why do I need python3.9 to link the rust crate?

[bminixhofer]

Why do I need python3.9 to link the rust crate?

I'm not sure; maybe because of the Python bindings? They're in the same workspace.

Looks good to me now! Thanks a lot!