Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

browse() returns an error when not interactive #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

salvafern
Copy link

Update documentation of rerddap::browse()

Description

The docs of rerddap::browse() indicate that, if interactive() = FALSE, then the function returns the URL of the dataset, but in fact it returns an error. I just quickly updated the docs.

Related Issue

Found out by running rerddap on jupyter notebooks, relates to jupyter/notebook#5151

@rmendels
Copy link
Collaborator

rmendels commented Dec 7, 2024

@salvafern Thank you for the heads up. I have been away from work, so am just looking at this. If you look at all of the documentation in several places it says

#' Note that it is an error to call this when `base::interactive()`
#'' returns `FALSE`

However it does say below that:

#' @return if in interactive mode, opens a URL in your default browser; 
#' if not, then prints the URL in the console

I haven't checked this but looking at the code I believe that is not true, as you state, However I don't think the solution is to change the docs, I believe the solution is to change the code to do as it states as you don't want code to finish that "ungracefully". I believe I have the fix for that but haven't tested it yet. I am planning a new release of 'rerddap' for some new features in 'rerddap::tabledap()' as well as how I test that the base ERDDAP URL is valid, and will include the fix in that, but I will be in and out of work till after the new year so unsure of a timetable (I try to be a good CRAN maintainer by doing a lot of tests before submission).

I also would point out that in the link you gave it is stated that "This is more of an [IRKernel (https://github.com/IRkernel/IRkernel) i" and it would be nice if this feature worked interactively in notebooks. I need to see what happens in R notebooks and Quarto docs, but either way I need to fix this.

@salvafern
Copy link
Author

Hi @rmendels, indeed the best would be that the function returns the URL in case the session is not interactive - I just wanted to do a quick PR so it is not forgotten. But having a second look I wonder if this behavior was on purpose since utils::browseURL() fails silently if you try to run it in a non-interactive session.

In any case up to you and thanks a lot for maintaining rerddap! I am currently using it to access Bio-Oracle v3 data, and I will showcase how to access EMODnet data products with rerddap in this webinar -> https://eudata4oceandecade.eu/ (code). Let me know if I can help.

@rmendels
Copy link
Collaborator

rmendels commented Dec 9, 2024

@salvafern Thanks for the comments - we are firm believers that the data people need should be readily available from the tools they prefer to use. Yes we are aware of the Bio-Oracle ERDDAP, a very well done ERDDAP in our opinion, plus very nice dataset(s). And that talk looks very interesting I will try to catch it.

@rmendels
Copy link
Collaborator

@salvafern - not tested extensively but if you feel comfortable replacing an 'rerddap' function replace what is in browse.R with:

browse <- function(x, url = eurl(), ...){
  UseMethod("browse", x)
}


#' @export
browse.character <- function(x, url = eurl(), ...){
  if (missing(x)) stop("datasetid is required")
  uri <- sprintf(paste0(url, 'info/%s/index.html'), x)
  
  if (interactive()) {
    utils::browseURL(uri)
  } else {
    message("URL: ", uri)
    return(uri)
  }
}

#' @export
browse.info <- function(x, url = eurl(), ...){
  datasetid <- attr(x, "datasetid")
  browse(datasetid, ...)
}

#' @export
browse.tabledap <- function(x, url = eurl(), ...){
  datasetid <- attr(x, "datasetid")
  browse(datasetid, ...)
}

#' @export
browse.griddap_nc <- function(x, url = eurl(), ...){
  datasetid <- attr(x, "datasetid")
  browse(datasetid, ...)
}

#' @export
browse.griddap_csv <- function(x, url = eurl(), ...){
  datasetid <- attr(x, "datasetid")
  browse(datasetid, ...)
}

#' @export
browse.default <- function(x, url = eurl(), ...){
  att <- attributes(x)
  if (!is.null(att)) {
    if ('datasetid' %in% names(att)) browse(att, ...)
  } else {
    stop(sprintf("browse method not implemented for %s.", class(x)[1]),
         call. = FALSE)
  }
}

for example:

Screenshot 2024-12-09 at 4 10 06 PM

I need to check why the double-print but that is Jupyter notebook running R and not throwing an error. I think I am going to try and work on getting a release to CRAN this week, will see how it goes.

@rmendels
Copy link
Collaborator

@salvafern rerddap release candidate 1.2.0 is on Github. It should contain the fix so that the URL is returned. It allows tabledap() to use parquet files for download which are much smaller, if the ERDDAP server is at least version 2.25. I likely will submit to CRAN tomorrow. I just like to sit on it for a day to make certain of things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants