Rustisms: part 1

[tombh]

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.

Date:   Thu Dec 8 10:16:25 2022 -0300
    ci: Remove `cargo check` stage

    `cargo clippy` is already a super set of `cargo check`

Date:   Thu Dec 8 10:37:37 2022 -0300
    chore: Introduce `cargo stele` for dev/CI utils

    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

Date:   Thu Dec 8 12:03:10 2022 -0300
    chore: Refactor linting into `lib.rs`

    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.

Date:   Thu Dec 8 12:23:25 2022 -0300
    chore: Remove linting definitions from `.vscode`

    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.

Date:   Thu Dec 8 12:33:55 2022 -0300
    chore: Remove `#[inline]`s

    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.

Date:   Thu Dec 8 13:16:13 2022 -0300
    chore: Move integration-style tests to tests/

    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.

Date:   Thu Dec 8 13:34:04 2022 -0300
    chore: Good example of `Err` to `?` refactor

    This is a good example of exactly why `?` can be so powerful.
    `Err() => continue` or `Err() => return` are essentially synonyms
    for `?`

Date:   Thu Dec 8 19:32:38 2022 -0300
    chore: Top-level function to match anyhow errors

    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()`.

Date:   Thu Dec 8 21:53:27 2022 -0300
    chore: Use `Into` trait for known and basic types

    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

Date:   Thu Dec 8 21:58:51 2022 -0300
    chore: Use import alias `as`

    This is just like Python

Date:   Thu Dec 8 22:19:32 2022 -0300
    chore: Use `String` matchers rather than array[]

    Generally speaking I think array[] access is only used in very
    perofrmance critical applications.

[tombh]

Strange that CI is failing with error: missing trait method provided by default:__private_get_type_id__``, and I can't see the error locally. It must some tool versioning going on, like mismatching Clippy versions or something. I'll see if I can figure it out now, cos one all the versions have been set, this CI/local divergence should never happen again.

[tombh]

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]

So the CI error was caused by the Github Cargo action using the latest rustc version v1.66. Once I installed that version locally I could reproduce the lint error. So I've now pinned the minimum Rust version to 1.66. This is not at all a requirement. There doesn't seem to be a way to tell Cargo to support a max version. I believe compatibility is handled simply by specifying the "stable toolchain" which is the default unless explicitly specified.

I'm not sure what the best approach here is. For example one approach is to pin the Github action to a specific rustc version. But whilst we haven't identified any hard requirements on a specific version yet, maybe it's good to let the Github action automatically use the latest version for each run?

[tombh]

I was just about to rebase this now, but I've got to take a call! Be right back...