Equal assign r36 bug #6
Conversation
`getParseData()` has a new `token` type in R3.6: `equal_assign`. `xml_parse_data(parse(text = "a = 1", keep.source = TRUE))` was returning invalid xml when ran in R 3.6. A test that captures this was added.
Expressions can be nested inside each other in the data returned by
getParseData() in such a way that the end position of the nested
expressions are identical.
For example, `equal_assign` and `expr[2]` end at the same position
in the following (relevant in R3.6).
- equal_assign:
- expr[1]:
- SOME_SYMBOL
- EQ_ASSIGN
- expr[2]:
- SOME_VALUE
xml-termini for `equal_assign` and both `expr` nodes are added
while running xml_parse_data; but these termini were originally
sorted such that that for `equal_assign` came before the second
`expr`. Like: `<equal_assign>...<expr[2]></equal_assign></expr[2]>`.
The xml-termini for each expression are now sorted by the start
position of the expressions (in addition to their other
coordinates).
|
The CI failed for R-3.1 but passed for all other releases. I don't think I'll be able to get it to pass CI on R 3.1 since covr ('suggest'ed by xmlparsedata) imports httr (which requires R >= 3.2). Is there a way to run the CI for R 3.1 such that only the xmlparsedata 'import's are required? |
|
@russHyde, we can drop 3.1 compatibility. |
R/package.R
Outdated
| ## - the terminal nodes from pd2 may be nested inside each other, when | ||
| ## this happens they will have the same line1, col1, line2, col2 and | ||
| ## terminal status; and 'start' is used to break ties | ||
| ord <- with(pd, order(line1, col1, -line2, -col2, terminal, -start)) |
There was a problem hiding this comment.
In general using with() in packages is not recommended, mainly because it will generate a NOTE about global variables, which you then have to whitelist with utils::globalVariables(). I think you should convert this to
pd <- pd[ order(pd$line1, pd$col1, -pd$line2, -pd$col2, pd$terminal, -start), ]There was a problem hiding this comment.
Will do (but will use -pd$start)
There was a problem hiding this comment.
Mostly looks good, I made a small suggestion inline.
Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).
|
Thanks @jimhester . Would you be happy for me to move the new test into test/testthat/test.R and drop the test-debug...R file? (I'll reduce the comment volume within the new test as well). |
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 58 56 -2
=====================================
- Hits 58 56 -2
Continue to review full report at Codecov.
|
|
Have made the requested changes. Regards |
|
Thanks much! |
Following up on a bug in R3.6 #5
that meant
xml_parse_data(parse(text = "a = 1"))couldn't be convertedto valid xml (although this could be parsed in R3.5 using
xml_parse_data).This appears to result from changes to
getParseDatain R3.6.[not sure I've explained the problem / fix particularly well; apologies]