Nice work @theRookieCoder! It's always nice to see people creating cool stuff in Rust 
Here are some thoughts I had while skimming your code. A lot of them are stylistic points, so feel free to disregard them if you feel differently 
I also haven't checked whether any of my suggestions compile.
use labrinth::funcs::*;
use octorok::funcs::*;
use readme::README;
use reqwest::Client;
use std::fs::{File, OpenOptions};
use std::io::Write;
use std::process::exit;
use util::{cli::*, json::*, wrappers::*};
Try to avoid glob imports.
I wanted to see how you had implemented command-line argument parsing and it took me a couple seconds to figure out that get_subcommand()
probably came from the cli
module. More seriously, I had no idea where get_config_file()
came from at all and resorted to cloning the repo and searching for it locally because it was too hard looking for it on GitHub 
Normally I would either explicitly import the get_subcommand()
function (i.e. use util::cli::get_subcommand
) or import the module and refer to it as cli::get_subcommand()
. That way you aren't polluting your namespace and people can immediately see where things come from.
SubCommand::Help { implied } => {
println!("{}", README);
if implied {
exit(126);
}
}
If you don't have a help
subcommand clap
will actually generate one for you! That lets you drop the readme
module and means you can't have issues where your help text and command-line args are out of sync because you added a new subcommand/flag and forgot to update readme.rs
.
From clap
's docs:
If you are using subcommands, clap
will also auto-generate a help
subcommand for you in addition to the traditional flags.
I haven't used clap
directly for a while now (I prefer structopt
's declarative style) but I believe it'll also do the same with version
.
print(&format!("Adding repo {}/{}...", owner, repo_name));
This looks a bit strange. I probably would have made print()
either accept any msg: impl Display
so you can get rid of that &
, or even declared my own macro which does the print!()
call and automatically flushes stdout
. - that way you can get rid of the leading format!
bit and write something like print_now!("Adding repo {}/{}...", owner, repo_name)
.
You might also want to use the log
crate instead of printing directly to the console.
pub async fn request(client: &Client, url: &str, relative: bool) -> Response {
let url: String = if relative {
// If provided URL is specified as relative, then prepend the base url
format!("http://api.github.com{}", url).into()
} else {
// Else directly assign the URL
url.into()
};
...
}
This relative
flag kinda bugs me because when I see the function being called I'm always going to need to check the source to see what the true
or false
refers to.
You could create a relative_request()
function which lets you replace something like this...
request(client, &format!("/repos/{}/{}", owner, repo), true)
... with this:
relative_request(client, &format!("/repos/{}/{}", owner, repo))
Alternatively, you could over-engineer things by making the request()
function always accept absolute URLs and then defining your own URL builder type/functions.
request(client, urls::github_repo(owner, repo))
pub mod funcs;
pub mod structs;
I think funcs
and structs
are pretty useless names for a module.
At the top of src/octorok/structs.rs
you even say
/*
* This file contains typed structs of the data structures used by the GitHub API
*/
What about calling it api_responses.rs
?
You should also use doc-comments instead of normal comments. Normal comments are completely ignored by the compiler, whereas if you use doc-comments it'll appear in the pretty HTML documentation you get from cargo doc
.
//! Data structures used by the GitHub API.
// src/octorok/structs.rs
use std::vec::Vec;
This import is unnecessary because Vec
is accessible by default (it's in the prelude).
Another "pattern" I've seen you doing is to define functions that accept strings as &str
(which is good!) but then pretty much every time they are used it's with temporary strings that are allocated at the use site with &format!(...)
.
Looking back at our request()
function:
pub async fn request(client: &Client, url: &str, relative: bool) -> Response { ... }
request(client, &format!("/repos/{}/{}", owner, repo), true)
This looks kinda weird and you actually end up doing a bunch of string copies.
For example, you'll have one copy of the string in the calling function (the format!()
call), then at the top of request()
we call into()
on the &str
we passed in to turn it into another heap-allocated String
, then we pass &url
to client.get()
which actually does another copy because the IntoUrl
implementation for &str
calls Url::parse()
and Url
contains a String
with the parsed URL.
If you look at reqwest::Client
's get()
method you can see it actually accepts any url: impl IntoUrl
, so you could avoid all those string copies by using helper functions which directly construct the right Url
.
You should also look into proper error handling. It looks like whenever you encounter an error you'll just emit a print then immediately std::process::exit()
.
/// Get a mod using the `mod_slug`, which can also be the mod ID
pub async fn get_mod(client: &Client, mod_slug: &str) -> Mod {
match request(client, &format!("/mod/{}", mod_slug), true)
.await
.json()
.await
{
Ok(typed) => typed,
Err(e) => {
println!("JSON deserialisation failed due to {}", e);
exit(122);
}
}
}
Calling std::process::exit()
directly is undesirable for a couple reasons:
- People won't be able to use this as a library because the moment anything unexpected occurs (e.g. a URL doesn't exist) the entire program will crash, and they've got no way of preventing it
- Terminating the program immediately is kinda like stopping a computer by pulling out the power plug/battery. Sure it'll stop the program, but destructors won't be called meaning the
Client
won't have a chance to gracefully close connections and data written to open files won't be flushed to disk (i.e. potential data loss)
You also hard-coded the exit code. At the very least you'll want to move them to constants with doc-comments so I know the difference between an exit code of 122
and 124
.
When I say "proper error handling" I mean that your functions should return a Result
and use ?
. If you introduce a custom error type that'll also let you differentiate between the various types of errors and exit with the correct error code at the very top of the program.
#[tokio::main]
fn main() {
match run().await {
Ok(_) => { /* exit successfully */ },
Err(MyError::ThingDoesntExist(thing)) => {
println!("Error: \"{}\" doesn't exist", thing);
std::process::exit(122);
},
Err(MyError::ReqwestError(e)) => {
println!("HTTP request failed: {}", e);
std::process::exit(124);
},
...
}
}
async fn run() -> Result<(), MyError> {
// contents of the previous `main()` function.
}
enum MyError {
ThingDoesntExist(String),
ReqwestError(reqwest::Error),
...
}
You may also want to look into the anyhow
or thiserror
crates.
Your get_config()
function can actually accept anything implementing std::io::Read
, not just a File
.
I'd also argue you are doing something more than just "getting" the config, you are parsing it from a string representation.
use std::io::Read;
pub fn parse_config<R: Read>(reader: R) -> Config {
// Read file contents
let mut contents = String::new();
match reader.read_to_string(&mut contents) {
Ok(_) => (),
Err(e) => {
println!("Could not read file due to {}", e);
exit(120);
}
}
// Try deserialising contents and return it if successful
match from_str(&contents) {
Ok(config) => config,
Err(e) => {
println!(
"Config file deserialisation failed due to {}. Check that the formatting is correct!",
e
);
exit(120);
}
}
}
But it gets better. Turns out serde_json
has a from_reader()
function which accepts a reader directly without the temporary string!
use std::io::Read;
pub fn parse_config<R: Read>(reader: R) -> Config {
match serde_json::from_reader(reader) {
Ok(config) => config,
Err(e) => {
println!(
"Config file deserialisation failed due to {}. Check that the formatting is correct!",
e
);
exit(120);
}
}
}
If we use the error-handling suggestions from earlier we can also get rid of the match
statement.
use std::io::Read;
pub fn parse_config<R: Read>(reader: R) -> Result<Config, serde_json::Error> {
serde_json::from_reader(reader)
}
And now we can see the parse_config()
function wasn't needed at all. You could have just done this in main()
:
let config: Config = serde_json::from_reader(config_file)?;
(assuming main()
returns a Result<(), SomeErrorType>
where you have defined SomeErrorType
to have a From<serde_json::Error>
impl)
You shouldn't be using string operations to create paths.
let config_file_path = format!("{}{}", config_file_dir, "config.json");
let config_file_path = Path::new(&config_file_path);
Instead, the Path
type has a join()
method which is often a lot nicer and side-steps a lot of issues you can have when dealing with filesystem paths (/
vs \
, invalid characters, etc.).
let home = tilde("~");
let config_file_path: PathBuf = home.join(".ferium").join("config.json");
Your write_to_config()
function does a bunch of set_len()
and seek()
's. If it were me I'd just pass in the config file's path and use File::create()
to truncate the file to get something close to this:
pub fn save_config(config_file: &Path, config: &Config) -> Result<(), SomeErrorType> {
let f = File::create(config_file)?;
serde_json::to_writer_pretty(f, config)?;
Ok(())
}