Closes #61 Check MD5 regardless of quiet flag value

[enriquemondragon]

Closes #61 Check MD5 regardless of quiet flag value

[e-kotov]

@enriquemondragon great job on deduplication!

Two points:

1. There is one new problem introduced. When you run:

x <- java_download(8, quiet = FALSE, force = FALSE)
# or
x <- java_download(8, quiet = FALSE, force = TRUE)
# or any other combination of arguments

And the file does not yet exist, the function will not download the file and no md5 check will be performed.

2. Regarding the alert in case of failed md5:

For every cli::cli_inform line, I'm checking that quiet == FALSE, except for:

cli::cli_alert_danger("MD5 checksum mismatch. Please try downloading the file again.", .envir = environment())
Because I thought this would be something that the user would like to know, even if the quiet flag was set to TRUE. Let me know what you think!

I would argue that it is actually best to throw an error and stop, rather than just a warning, as broken distribution file will lead to problems during install and use of Java, so it's best to fail.

[enriquemondragon]

@e-kotov thank you!

I explicitly wrote down all the if cases and created a function called download_dist_check_md5 to address deduplication in this new scenario. Both cases seem to work well. I added the function at the beginning of the script, let me know if I should make any improvements.

Edit: I just saw that one test failed, I'll check this

[e-kotov]

Edit: I just saw that one test failed, I'll check this

yep, it's because of the md5sum and md5sum_expectedm

# Download distribution and check MD5 checksum
  download_dist_check_md5 <- function(url, dest_file, md5sum, md5sum_expectedm, quiet) {

    curl::curl_download(url, dest_file, quiet = FALSE)
    curl::curl_download(url_md5, dest_file_md5, quiet = TRUE)

    if (!quiet) {
      cli::cli_inform("Download completed.", .envir = environment())
    }
    md5sum <- tools::md5sum(dest_file)
    md5sum_expected <- readLines(dest_file_md5, warn = FALSE)

    if (md5sum != md5sum_expected) {
      cli::cli_alert_danger("MD5 checksum mismatch. Please try downloading the file again.", .envir = environment())
      unlink(dest_file)
      return(NULL)
    } else {
      if (!quiet) {
        cli::cli_inform("MD5 checksum verified.", .envir = environment())
      }
    }
  }

perhaps you don't need these args at all as you overwrite them anyway in lines:

md5sum <- tools::md5sum(dest_file)
md5sum_expected <- readLines(dest_file_md5, warn = FALSE)

[enriquemondragon]

Thank you, I think this has been corrected. I also removed the redundant args

[e-kotov]

@enriquemondragon I replaced the warning with cli_abort, so that the functions stops in case of MD5 check fail. Otherwise, the user may miss the error and unknowingly continue with the corrupted distribution.

[e-kotov]

@enriquemondragon all done now! Great, thanks for your input!