ia-get feedback and request to contribute PRs

[DanielDewberry]

Hi Martin,

We met in Riga and sat in the hotel bar with Popey and others until the early morning talking about Ubuntu, Ubuntu Hideout, Bottom, Danger Brothers, and the other exploits of Rik Mayall and Ade Edmondson. It was lovely to meet you both and share a few laughs. (This night )

Feedback

Regarding: Linux Matters Episode 20

The application itself looks good! I appreciate that your are conducting an experiment on the merits of AI Driven Development :TM: but since you mentioned contributions on Episode 20 of Linux Matters I thought I would offer these thoughts. They might be of interest to you to see where an external programmer can spot contextual/ ecosystem deficits in the generated answers.
I would like to create a PR for as many of the following things as you are comfortable allowing contributors address.

Documentation

I would like to contribute comment-documentation for the functions, structures and Rust module which will be converted into proper Rust documentation when cargo doc is executed.

is_url_accessible error management

is_url_accessible returns a Result, but no error is ever propagated from the function - only Ok(true) or Ok(false). This means that error handling here and here never reaches the Err branches.

This can be corrected as follows:

async fn is_url_accessible(url: &str) -> Result<bool, Box<dyn Error>> {
    let client = reqwest::Client::new();
    let response = client.get(url).send().await?;  // ? will propagate errors
    Ok(response.status().is_success()) // If your goal is to return `true` and `false` as Ok, then this will work.
}

.is_success() checks for HTTP response code 200-299 so I suspect you actually would want to handle the false case as an error as well e.g:

async fn is_url_accessible(url: &str) -> Result<bool, Box<dyn Error>> {
    let client = reqwest::Client::new();
    let response = client.get(url).send().await?;
    response.error_for_status()?;  // Propagate an error if HTTP response code is 400 - 599
    Ok(true)  // Only return Ok if no transport or HTTP error is encountered.
}

• This will correct some currently hidden errors
• Your existing error handling logic at the call-sites will be compatible with this although further updates can be made to improve the function return semantics:

async fn is_url_accessible(url: &str) -> Result<(), Box<dyn Error>> {
    let client = reqwest::Client::new();
    let response = client.get(url).send().await?;
    response.error_for_status()?;
    Ok(())
}

The call-sites then manage the errors by matching Ok(()) or Err(e) e.g.

    match is_url_accessible(details_url).await {
        Ok(()) => println!("╰╼ Archive.org URL online: 🟢"),  // Ok(true)/ Ok(false)/Err(e) semantics replaced by Ok(())/Err(e)
        Err(e) => {
            println!("├╼ Archive.org URL online: 🔴");
            panic!  ("╰╼ Exiting due to error: {}", e);
        }
    }

and

    match is_url_accessible(&xml_url).await {
        Ok(()) => println!("├╼ Archive.org XML online: 🟢"),
        Err(e) => {
            println!("├╼ Archive.org XML online: 🔴");
            panic!  ("╰╼ Exiting due to error: {}", e);
        }
    }

is_url_accessible request type

Instead of performing a GET request, it might be better internet citizenry to perform a HEAD request since no response body is required.

    let response = client.head(url).send().await?;

Test cases for PATTERN

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn check_valid_pattern() {
        let regex = Regex::new(&PATTERN).expect("Create regex");
        assert!(regex.is_match("https://archive.org/details/Valid-Pattern"))
    }

    #[test]
    fn check_invalid_pattern() {
        let regex = Regex::new(&PATTERN).expect("Create regex");
        assert!(!regex.is_match("https://archive.org/details/Invalid-Pattern-*"))
    }
}

PATTERN syntax

The escape sequences \/ are unnecessary and can be replaced with / only:

static PATTERN: &str = r"^https://archive\.org/details/[a-zA-Z0-9_-]+$";

Valid sequences can be seen here.

The aforementioned test cases demonstrate this works.

Request user-agent

It might be considered good internet citizenry to set the application user-agent to IA-Get for requests.

Packaging

I would like to help create a snapcraft.yaml. I have an existing file which I should be able to recraft.

---

I hope these thoughts are received in the good spirit they are intended and if you decide to incorporate any of these you would permit me to submit the commits for them.

Best regards

Daniel

[flexiondotorg]

This issue is ace, and a very useful contribution to the experiment 🧪 I will get around to including the fixes/improvements you've suggested, in a week or so 👍

Also, it was ace to meet you in Riga 🍻