Rustisms: part 1 #5
Conversation
|
Strange that CI is failing with |
|
Ok, so the only remaining thing is to figure out what's going on with the lint error on CI. It could be easily ignored, but it's worth me going over all the places where version pinning is needed, so that we don't get this divergence again. |
tombh
force-pushed
the
tombh/rustism-first-review
branch
2 times, most recently
from
9179c2d to
6f6415a
Compare
|
So the CI error was caused by the Github Cargo action using the latest I'm not sure what the best approach here is. For example one approach is to pin the Github action to a specific |
tombh
force-pushed
the
tombh/rustism-first-review
branch
3 times, most recently
from
df6cfb8 to
00042a8
Compare
See the comments at the top of `./cargo-stele` for more details. Basically, this is just an example. `just`[1] is probably better. 1. https://github.com/casey/just
See `lib.rs` for detailed explanation. The idea here is to centralise lint requirements as much as possible. Avoiding unnecessary repetition. Perhaps it could be thought of as a bit like the "cascade" in CSS. There's a base "pendantic" requirement for as much lint feedback as possible. Then crate-wide exceptions are made. Then file-wide exceptions are added. And finally function-specific exceptions are made on a case by case bases. Note that by default lints will just produce warnings. Only in CI will warnings be converted to failures.
Lints are now formalised in the source code itself. But it's still good to give VSCode users the headstart of defaulting to Clipp over `cargo check`. I added a `etc/NOTES.md` to keep a record of the pendantic VSCode settings. I personally use something like as my default Neovim config. But having a `etc/NOTES.md` might not be something you want.
Copied comment from `lib.rs` linting: Although performance is of course important for this application, it is not currently such that it would benefit from explicit inline suggestions. besides, not specifying `[inline]` doesn't mean that a function won't be inlined. And if performance does start to become a problem, there are other avenues to explore before deciding on which functions would benefit from explicit inlining.
Any tests amongst src/* files are generally unit-style tests. Of course this isn't a strict requirement, just Rust convention. The fact that these tests depended on fixtures made it quite clear that they are integration tests.
This is a good example of exactly why `?` can be so powerful. `Err() => continue` or `Err() => return` are essentially synonyms for `?`
This demonstrates the "unpacking" of the potentially diverse
automagically collected anyhow errors assocaited with finding and
getting a Git blob. See `blob_error_response()`. Internal errors are
safely matched to the appropriate user-facing HTTP responses.
Note the use of the helpful `anyhow::bail!` macro that can be used
in place of `Err(anyhow::anyhow!("error message..."))`.
Also note how `async fn get_blob()` only needs to match for `Err` in
just one place because of generous use of `?` in the refactored `fn
find_blob()`.
Often you can just lean on Rust's type system and built-in typecasting functionality. See: https://doc.rust-lang.org/rust-by-example/conversion/from_into.html
Generally speaking I think array[] access is only used in very perofrmance critical applications.
All HTTP requests are now logged with useful data. And other traces, debugs, infos, etc, that are logged during the lifetime of a HTTP request are also prepened with the that same HTTP context data. Also an example of adding custom request data and conditionally acting on that data is included. Namely, adding request duration and logging a warning when the duration is over a certain amount of time.
I needed this to manually test the tracing. So I tidied it up and made it the basis for more formally handling Stele-specific errors.
Hopefully fixes missing trait lint error
tombh
force-pushed
the
tombh/rustism-first-review
branch
from
00042a8 to
4ea9e4e
Compare
|
I was just about to rebase this now, but I've got to take a call! Be right back... |
This is my first review of the codebase. There may seem to be a lot of feedback, but it is in fact all merely referring to idiomatic code and known conventions. The design and quality is already very good, and could reasonably be shipped as-is.
In this particular case I've made the format of the review as a series of commits, each commit attempting to introduce a single new idiom or convention. I've pasted the Git log here for ease of reading. The commits are in reverse order, therefore the oldest appears first. Also, where possible, I made the commits in order of significance, therefore the most important and idiomatic changes appear first. Though don't take that too literally, as sometimes I needed to make less significant preparatory commits to allow the following commit to be usefully isolated.
I would recommend clicking on each commit, reading its message and viewing the diff. Like that it should be hopefully possible to see the single idea I'm introducing in each step.