Early prototype of CLI client for a REST API

code review
1

Ok, if anyone (esp. familiar with clap for building a CLI and reqwest for making HTTP requests) has a few minutes to give me some feedback on my code that would be very welcome! You can find it here: https://codeberg.org/jezcope/doitools

Once I've implemented more than a couple of commands the intention is to factor out the details of accessing the relevant APIs and then make that available as a library as part of the crate too. Especially interested in the following:

  • Any obvious missed rust-isms (e.g. where I've done something weird to defeat the borrow checker)
  • Does the way I've structured the code make sense? I have a tendency to over-engineer just in case...
  • How do you add a test suite when depending on an external API and a network connection?
2

Some observations in no particular order:

  • In datacite.rs, you set a custom user agent, but you've already set the user agent in config.rs.

  • Aside from checking for 404's in datacite.rs, you never check whether the HTTP responses you receive have a successful status code. One way to automatically convert failed status codes to Errs is with Response::error_for_status().

  • In commands.rs, you can simplify use crate::commands::... to use self::....

  • If you're distributing both a library crate and a binary crate in the same package (as opposed to, e.g., separate packages in a workspace), any dependencies that are specific to the binary (e.g., clap) should be behind a feature so that users of just the library can avoid pulling them in.

  • If you change DataCiteArgs::run() to take self rather than &self (like you're already doing for DoiArgs::run()), you can get rid of the ref in the match pattern and pass String rather than &String to fetch_doi() (though, when you start to make fetch_doi() a library function, it would be better for it to take &str — or even a parsed Doi type — instead).

  • In doi.rs, you should be able to simplify this:

        .get(self.format.as_str())
    .unwrap_or(&self.format.as_str())

    to:

        .get(&self.format)
    .unwrap_or(&self.format)

Does the way I've structured the code make sense? I have a tendency to over-engineer just in case...

Aside from the fact that all the functionality in the library crate is currently just Clap commands, without any way to, say, call fetch_doi() without instantiating one of those commands — which I'm assuming is more due to this being an early prototype rather than a design you're actually going to stick with — it seems fine to me for now. My biggest reservations are:

  • A proper library should use structured error types (The thiserror crate can help here) rather than stuffing everying in anyhow.

  • Instead of making CONFIG a global variable and panicking if the client can't be constructed, I would recommend adding a Config::new() -> Result<Config> method that returns an error if client construction fails, and then your main() or any library user should call this method and pass the Config to the types & functions that need it.

How do you add a test suite when depending on an external API and a network connection?

If you're lucky, the API in question supports running its server locally (e.g., in a Docker container), in which case you can spin up a local instance of the API for use during tests. The next best option would probably be to just make actual requests to the API during testing, but then you need to guard against tests failing due to network errors and such, which I don't know how to do in Rust.

1 Like
3

Ah, this is really helpful, thank you so much!

4

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.