8000 add ... to xml_structure and descendents, closes #244 by MichaelChirico · Pull Request #247 · r-lib/xml2 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add ... to xml_structure and descendents, closes #244 #247

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

8000
Closed
wants to merge 2 commits into from

Conversation

MichaelChirico
Copy link
Contributor
@MichaelChirico MichaelChirico commented Feb 12, 2019

#244

This is the simplest implementation but IMO not ideal:

  • A bit awkward about sep. Maybe make sep an argument as well? Seems the simplest way to catch duplicated sep at the outset. Otherwise will have to look at match.call? But I wasn't sure whether to only put a test for sep at the top (in xml_structure and html_structure?l @exported functions? Whenever cat shows up?)

  • Seems a bit odd that you have to remember to supply append = TRUE... seems reasonable to expect a call to xml_structure to make only one file. And the current API means if your desired behavior is to overwrite an existing file with the full output of xml_structure (not a particularly odd behavior IMO), you're out of luck (or at least need to do some more manual stuff; I don't think a lazyeval trick with append = file.exists(file) will help since I think it will only evaluate once, not at each call?)

@jimhester
Copy link
Member

Thank you for working on the pull request! I ended up implementing this in a different way, exposing the file parameter rather than passing the dots (672e3f6) as that avoids the issues you described dealing with sep and append.

I did use the test from this PR, so thank you for including that!

@jimhester jimhester closed this Jul 25, 2019
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
0