search

ropensci / RNeXML

Implementing semantically rich NeXML I/O in R
https://docs.ropensci.org/RNeXML
Other
13 stars 9 forks source link

Fixes NAs in containing element IDs for nested meta #210

Closed hlapp closed 5 years ago

hlapp commented 5 years ago

Also includes a (pretty thorough) test case.

hlapp commented 5 years ago

The failure on Circle CI seems to be because we're CPU time limited there now? Of note, this here does not yet include #199, where we've first seen the Circle CI failure, at a different position. The last merged PR on this branch is #206, which passed everywhere.

Maybe it's worth reconfiguring how the manuscript rebuild on Circle CI gets triggered. Maybe this shouldn't need to be done for each pull request and each update to it, but only when master changes? Given the shape of the test suite plus the vignettes, it's hardly imaginable what kind of problem the manuscript code would detect that the tests and vignettes missed.

Perhaps I should turn this into an issue?

hlapp commented 5 years ago

Rebased #212 into this branch so we get codecov results as well.

cboettig commented 5 years ago

These looks good to me, nice test suite too. Looks like the branch is conflicted now though, maybe I wasn't careful about the merge order this round(?)

hlapp commented 5 years ago

Conflict fixed. Want me to go ahead with merge, or do you want to re-review?

cboettig commented 5 years ago

Ready to merge when you are

On Fri, Nov 23, 2018 at 9:14 AM Hilmar Lapp notifications@github.com wrote:

Conflict fixed. Want me to go ahead with merge, or do you want to re-review?

— You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub https://github.com/ropensci/RNeXML/pull/210#issuecomment-441289328, or mute the thread https://github.com/notifications/unsubscribe-auth/AANlejs8Scvn99M79iUdeDzYJEJbzHYIks5uyCzpgaJpZM4Yvxi4 .

--

Carl Boettiger http://carlboettiger.info/