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

can't use a non-prefix? #8

Open
mdsumner opened this issue Feb 15, 2024 · 6 comments
Open

can't use a non-prefix? #8

mdsumner opened this issue Feb 15, 2024 · 6 comments

Comments

@mdsumner
Copy link

I'm not sure my understanding is correct, but I want to avoid the "d1" prefix and not have one:

uri <- 'https://dapds00.nci.org.au/thredds/catalog.xml'

thredds::CatalogNode$new(uri, prefix = "")$prefix
#> [1] "d1"
thredds::CatalogNode$new(uri, prefix = NA_character_)$prefix
#> [1] "d1"
thredds::CatalogNode$new(uri, prefix = NULL)$prefix
#> [1] "d1"
thredds::CatalogNode$new(uri, prefix = character(0))$prefix
#> [1] "d1"

?

@eblondel
Copy link
Collaborator

Hi @mdsumner initially the prefix had to be set manually, the reason why we still have it as legacy arg.

At some point I wrote some code then to inherit it from the xml namespace definition, no need to declare this prefix from user viewpoint. The prefix for 'http://www.unidata.ucar.edu/namespaces/thredds/InvCatalog/v1.0' namespace is inherited from xml2; If no specific (local) xmlns is given by Thredds, xml2 sets it to 'd1', that's the default behavior of xml2.

IMHO we should get rid of the prefix argument in the different functions, we just need it to perform the relevant Xpath behind the scene. There is no need as well to handle the namespace prefix as public field, and through the print method
Let's see what @btupper thinks of this.

@btupper
Copy link
Member

btupper commented Feb 16, 2024

@mdsumner your example reminds me of Henry Ford's quote ...

Any customer can have a car painted any colour that he wants so long as it is black.

I can't recall the circumstances that drove the need for handling prefix, and I would defer to @eblondel to guide us to the right solution which is to either remove the optional prefix handling as suggested or to fix it so the user can override the default.

If it the choice is to fix the behavior then I think the only place we need to address it is here. We would just have to test if the prefix argument is NULL before assigning the value.

Maybe something along the lines of ...

if (is.null(prefix)){
  self$prefix <- names(ns)[ns == "http://www.unidata.ucar.edu/namespaces/thredds/InvCatalog/v1.0"]
} else {
  self$prefix = prefix[1]
}

But if it really isn't necessary at all for the user to optionally manage it then I am fine with removing its exposure.

@mdsumner
Copy link
Author

ah ok, I need to explore more - but the immediate motivation is I couldn't get it to work with this thredds, so I picked on the first thing that seems to a blocker , and maybe that's not the real issue 🙏

@btupper
Copy link
Member

btupper commented Feb 18, 2024

In the meantime... might you try the fix-prefix branch and see it provides a short term solution?

remotes::install_github("BigelowLab/thredds", ref = "fix-prefix")

@mdsumner
Copy link
Author

that works:

thredds::CatalogNode$new(uri, prefix = "")

thanks, I'll keep exploring

@mdsumner
Copy link
Author

just fyi I can't make any sense of the xml I was trying to explore with this package, it doesn't work at all and I think their index is a bit broken, doesn't seem to be a problem in this package.

I will explore their stuff from the inside 🙏

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

No branches or pull requests

3 participants