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

fix-up R parser bug in get_source_expressions #1056

Merged
merged 2 commits into from
Apr 10, 2022
Merged

Conversation

MichaelChirico
Copy link
Collaborator

Split off from #1032 because in principle it could affect the logic whenever STR_CONST is involved (especially, it's probably needed as part of #1034)

AshesITR
AshesITR previously approved these changes Apr 9, 2022
@MichaelChirico MichaelChirico changed the title fix-up R parser bug in get_source_expressions WIP: fix-up R parser bug in get_source_expressions Apr 9, 2022
@MichaelChirico
Copy link
Collaborator Author

Adding a NEWS bullet and tests...

@MichaelChirico MichaelChirico changed the title WIP: fix-up R parser bug in get_source_expressions fix-up R parser bug in get_source_expressions Apr 9, 2022
@AshesITR
Copy link
Collaborator

Will this fix the xml as well? I suppose no, just wanted to confirm.

@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented Apr 10, 2022

Yes it will, because xmlparsedata builds from the parsed data.frame if we pass on.

Here we pass the data.frame:

tryCatch(xml2::read_xml(xmlparsedata::xml_parse_data(parsed_content)), error = function(e) NULL)

Here xmlparsedata builds off the data.frame it receives instead of running utils::getParseData() again:

https://github.com/r-lib/xmlparsedata/blob/ee6a15f0b9162e2d215635d9771dc5c09d3eacbf/R/package.R#L65

(this is why the other touch-ups, i.e. for tabs and equals signs, are also reflected on the XML tree we get back from xml_parse_data())

@MichaelChirico MichaelChirico merged commit f1643b7 into master Apr 10, 2022
@MichaelChirico MichaelChirico deleted the fix-octal branch April 10, 2022 08:02
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