search

Bioconductor / BiocCheck

http://bioconductor.org/packages/BiocCheck
8 stars 26 forks source link

BiocCheck should fail fast when the server does't return an HTTP 200 #2

Closed b-long closed 8 years ago

b-long commented 8 years ago

Rather than doing an early return at this line : https://github.com/Bioconductor/BiocCheck/blob/master/R/checks.R#L1133 , it would be better for the check to error (as it is a true error condition for the build system to not be authenticated). This function will not run if it's invoked from a user's machine, since the BIOC_DEVEL_PASSWORD environment variable is not defined.

As such, I'd recommend doing the following instead : 

stop("Unable to authenticate with mailing list: 'https://stat.ethz.ch/mailman/admin/bioc-devel'")

/cc @dtenenba @jimhester

b-long commented 8 years ago

Actually, that's wrong, since it's for any non-200 response it should message appropriately. I'll address that in my PR.

jimhester commented 8 years ago

See http://rpackages.ianhowson.com/cran/httr/man/http_condition.html for handling multiple response conditions.

But in this case just calling httr::stop_for_status() is probably fine.

b-long commented 8 years ago

Is there an advantage? Why not use something like this : 

stop(paste0("An error occurred communicating with mailing list: 'https://stat.ethz.ch/mailman/admin/bioc-devel'.  ","Server returned status: ",status_code(response)))

?

jimhester commented 8 years ago

Not really any advantage, your example seems fine

b-long commented 8 years ago

As mentioned on Trello, this bug is currently affecting the build of a new package (metagenomeFeatures) on perceval: http://bioconductor.org/spb_reports/metagenomeFeatures_0.99.3_buildreport_20150930114959.html#perceval_check_anchor