-
Notifications
You must be signed in to change notification settings - Fork 335
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
Switch from Rd_content/Rd_doc → Rd #125
Conversation
} | ||
|
||
# Recursively set classes of Rd objects | ||
set_classes <- function(rd) { | ||
if (is.list(rd)) { | ||
new_rd <- lapply(rd, set_classes) | ||
new_rd <- structure(lapply(rd, set_classes), class = class(rd)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could replace this entire block with
rd[] <- lapply(rd, set_classes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
Otherwise looks good (although you need a merge/rebase) |
oh this was fast! i just rebased |
set_class(new_rd) | ||
} else { | ||
set_class(rd) | ||
rd[] <- lapply(rd, set_classes) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that look good? (i added set_class(rd)
as return line)
but we have to add more tests first: i think i’ll have to think about this more… |
See #118 - I was thinking that a baseline set of regression tests would be to run staticdocs on itself and check into |
yes. my basic idea ATM is to only rely on all the added classes internally. everything exposed to the user (e.g. exported this removes the ability to print subsets of Rds to the console, though. I.e. user-facing methods return something like this, but also accept plain Rds and make sure internal functions get something like this:
alternatively i take back most changes, and just ensure that full this would be in line with your initial vision, but we’d need to make sure all user-facing methods are implemented for I.e. pretty much your current structure, only less buggy:
what do your think? |
Thanks for these ideas — I'm implementing at the moment as part of an almost complete rewrite of the html translation system |
OK, great! Then we can close this, right? |
also adds test and renames
parse_rd
toparse_topic
fixes #109