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

DO NOT MERGE: Optionally return new tags_diff column formatted for html rendering #498

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

barrettk
Copy link
Collaborator

@barrettk barrettk commented Jun 16, 2022

Putting this on hold for now, but keeping it open because we'll likely revisit before too long

closes #404

@barrettk
Copy link
Collaborator Author

barrettk commented Jun 16, 2022

Current implementation:

log_df <- bbr::run_log("inst/model/nonmem/basic")

log_html <- log_df %>% add_tags_diff(.format = "html") %>% 
  dplyr::relocate(tags_diff) %>% select(-c(absolute_model_path, yaml_md5)) 

log_html %>% knitr::kable(escape = F) %>% kableExtra::kable_styling() 

image

I added some notes to the docs. Specifically I wanted to highlight that you need to add escape = FALSE to the knitr call for this to work. If using a method to render html outside of knitr/kableExtra (dont know why you would), users would have to alter this. The tilda (~~) only works for rmarkdown text, and does not render in html

For reference, here's the actual returned table:

> log_html
# A tibble: 3 × 11
  tags_diff                                run         model_type description     bbi_args     based_on tags  notes  star  tags_added tags_removed
  <chr>                                    <chr>       <chr>      <chr>           <list>       <list>   <lis> <list> <lgl> <chr>      <chr>       
1 acop tag, other tag                      1           nonmem     original acop … <named list> <NULL>   <chr> <NULL> FALSE acop tag,… ""          
2 test threads; <s>acop tag, other tag</s> 1_2_threads nonmem     NA              <named list> <chr>    <chr> <NULL> FALSE test thre… "acop tag, …
3 test threads; <s>acop tag, other tag</s> 1_4_threads nonmem     NA              <named list> <chr>    <chr> <NULL> FALSE test thre… "acop tag, …

Note that I added an additional .collapse argument. I feel like users might appreciate that functionality, without necessarily wanting the additional column

@barrettk barrettk requested a review from seth127 June 16, 2022 18:46
@barrettk barrettk requested a review from kyleam June 17, 2022 16:59
@seth127
Copy link
Collaborator

seth127 commented Jun 17, 2022

@kyleam @barrettk you can pause on this for a moment. I'll take a look next week. This is gonna be a pretty specialized function and I need to think through the use cases a little more. For example:

  • Do we only intend it to be used in Rmd?
  • Does it need to be used with knitr?
  • Is there a difference based on whether we have results = 'asis' in the chunk options

@kyleam kyleam changed the base branch from develop to main June 22, 2022 15:54
@seth127 seth127 changed the title Optionally return new tags_diff column formatted for html rendering DO NOT MERGE: Optionally return new tags_diff column formatted for html rendering Aug 16, 2022
@kyleam kyleam removed their request for review August 16, 2022 18:54
@seth127 seth127 mentioned this pull request Feb 27, 2023
2 tasks
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.

format_tags_diff()
2 participants