waldronlab / cBioPortalData

Integrate the cancer genomics portal, cBioPortal, using MultiAssayExperiment
https://waldronlab.io/cBioPortalData/
30 stars 12 forks source link

Update cBioPortal.R #51

Closed ZWael closed 2 years ago

ZWael commented 2 years ago

Hi here i thing avoiding a character() function in the code is better for clarity Note that character("a12b3") results in an error Error in character("a12b3") : vector size cannot be NA/NaN token = put_your_token_here is also clear

i was using cBioPortalData 2.7.2 and the argument token=character("my_token") produced the error above

nice work

LiNk-NY commented 2 years ago

Hi @ZWael

Please don't confuse function defaults with inputs. If you have a token, use token = "my_token". See also, ?character

Best regards, Marcel

ZWael commented 2 years ago

Hi @LiNk-NY thank you for the reply, yes that was my point token = character() was somehow confusing for me I think changing

cBioPortal <- function(
    hostname = "www.cbioportal.org",
    protocol = "https",
    api. = "/api/api-docs",
    token = character()
    )

with

cBioPortal <- function(
    hostname = "www.cbioportal.org",
    protocol = "https",
    api. = "/api/api-docs",
      token = NA_character_
)

is more clear for newbies like me , and more consistent with the rest of code, character(1) or token = put_your_token_here are also clear alternatives

Best regards

LiNk-NY commented 2 years ago

Yes, this would look a bit more consistent with the rest of the package but the change creates an issue down the line because @Bioconductor/AnVIL validates the input at: R/Service.R:135: is.null(api_reference_headers) || .is_character(api_reference_headers)

> AnVIL:::.is_character(character())
[1] TRUE
> AnVIL:::.is_character(NA_character_)
[1] FALSE

The code in AnVIL would have to change to

AnVIL:::.is_character(NA_character_, na.ok = TRUE)

making this change is unlikely.