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

Change return type or behavior of as_xml? #69

Closed
amoeba opened this issue Oct 23, 2020 · 1 comment · Fixed by #70
Closed

Change return type or behavior of as_xml? #69

amoeba opened this issue Oct 23, 2020 · 1 comment · Fixed by #70

Comments

@amoeba
Copy link
Contributor

amoeba commented Oct 23, 2020

I'm so used to this that I forget that it might be confusing to new users. EML::write_xml returns NULL when you save to disk because of the behavior of emld's as_xml method returns the raw result from xml2::write_xml which returns invisible(). One of our team caught this over on NCEAS/datateam-training#229.

The relevant code is:

## Serialize to file if desired
if(!is.null(file)){
  xml2::write_xml(xml, file)
} else {
  xml
}

I wasn't sure what the best return type is here. The readr functions, which are common to most users, seem to return the input (to support piping). This method currently returns either an xml_document or NULL. Maybe the best thing is just to wrap the write_xml in invisible() and avoid making any big changes to the return type.

Thoughts @cboettig?

@amoeba
Copy link
Contributor Author

amoeba commented Oct 23, 2020

Oh, you know I just saw this in the docstring:

a xml_document object. Or if a file path is provided, the metadata is written out in XML file and the function returns NULL invisibly.

We're not currently doing this so I'll send in a PR.

amoeba added a commit to amoeba/emld that referenced this issue Oct 23, 2020
The docs indicate as_xml should return invisibly
when the file argument is specified but it
wasn't actually doing this.

Fixes ropensci#69
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 a pull request may close this issue.

1 participant