Closed alphastrata closed 2 years ago
I'll have to resolve the conflicts as I went and applied clippy suggestions between when you forked and the pull request. I did intentionally choose to do serial downloads, but only because of my inexperience with Rust async despite this sort of process begging for it. It will be good for my own education to study the changes you made post-merge.
I see!
Well, the good news is that async in rust is actually a LOT easier than in python (idk about Java..)
I suspect the largest number of clippy conflicts are going to idiomatic ones (as mentioned, I bumped the rust edition, to get the newest ones).
There is still much I'd love to chip away at across this, and your other rust repos -- Can certainly see how this and the last merge to sciimg are on the larger side, so, I'll try to be more selective on future work to avoid such a large number of changes etc.
Feature: Async downloads
Probably best summed up with a demonstration
Async
fetching from the server(s), why? because blocking requests are for the 90s. The above is powered byTokio
, an incredibly powerful and easy asynchronous framework.Error
handling withanyhow
, the premier library for handling errors in applications (there's another library by the same maintainer forErrors
in library code calledthiserror
)fetch
/get
functionality to avoid bloat on top of thereqwest
crate -- which has most of what you seem to be after builtin.async_trait
for theRunableSubcommand
, and in some places remove the constraint entirely (due to the amount of code I'd have to change to make it work both in this repo and insciimg
)tests
, removed some.Client
instantiation from one happening EVERY CALL to one single client that's reused for all requests (as the library designed it to be used).clippy
,fmt
, linting, compilation times, optimisations and so on...notes:
RunableSubcommand
is no longer theimpl
on a few of the calls toXYZ::run
, because of the difficulties implementingSend
/Sync
over a crate boundary and on a Trait (as opposed to anenum
/struct where it's easy.) This would mean changing code in thesciimg
crate, which is for the most part, well separated from this stuff. So the solution I've gone with (that I am not particularly fond of) is to keep all the methods but not have itimpl RunableSubcommand for XYZ
, but rather justimpl XYZ
. so all the methods are the same, which means no cross-cutting changes all over, but we loose the benefits of it being under the same trait bounds.async
.mru
bin.todo
refactor out all the places that're needlessly wrapping library code.
unify the error handling to be uniform across (i.e no more unwraps, anywhere!, using
anyhow!
and?
s, orthiserror
which can wraperror
types from other crates with convenient macros)style up the rest of the code-base, i.e there's lots of places we can have beautiful one-liners, this is particularly nice when we're using the
?
operator as it easily allows us to propagateerrors
up to the call site (which helps anyone looking at your app in future to fix bugs etc)there's a fair bit of ITM around, that's gotta go -- where possible create values without resorting to creating things you mutate.
there's a lot of duplicated code between files and such.. this should be factored out i.e remote.rs which is, named such in few places contains a large amount of the same code/calls/functions etc.
there are non-rusty
for
loops everywhere, which is almost certainly going be slower (assembly) thaniterators
so they have to go too -- lots of these algorithms to process the images look pretty trivial to parallelised withrayon
, and where not -- the work can be parallelised, or done on more efficient order(ing)s.this repo needs a contributor guide