insightsengineering / teal.reporter

Create and preview reports with Shiny modules
https://insightsengineering.github.io/teal.reporter/
Other
8 stars 9 forks source link

Don't keep code as a string #221

Closed pawelru closed 11 months ago

pawelru commented 1 year ago

Please change the below so that we don't keep executed code as a string: https://github.com/insightsengineering/teal.reporter/blob/b82c2acb63cf2ef80e1009bdc030c8bafce91bfe/R/Renderer.R#L38-L55

If string is really needed then please convert code object into a string

chlebowa commented 11 months ago

Would you prefer a function that is never called but whose body is pasted into the Rmd? That was the case during review but I didn't like having a function that is never called.

pawelru commented 11 months ago

IMHO language (or expression if it's simple enough) is better choice to store a code rather than string. String should be just representation of it (e.g. used by print method). This obviously depends on the use case - I haven't analysed that one yet. There is a big advantage of this being we don't hide code dependency. R CMD CHECK cannot interpret that code-string and detect package dependencies.

I didn't like having a function that is never called

I don't quite understand that. If it's not used why to keep it? Are you sure we are not calling it? https://github.com/insightsengineering/teal.reporter/blob/b82c2acb63cf2ef80e1009bdc030c8bafce91bfe/R/Renderer.R#L105

chlebowa commented 11 months ago

As you can see, the resulting string is a function definition statement. It is pasted into the resulting Rmd document and defined during rendering of that file. It is never called in the package itself.

I don't understand what you are linking to.

pawelru commented 11 months ago

I think it's called

https://github.com/insightsengineering/teal.reporter/blob/b82c2acb63cf2ef80e1009bdc030c8bafce91bfe/R/Renderer.R#L31-L82

This method is creates a .Rmd file and returns path to it

https://github.com/insightsengineering/teal.reporter/blob/b82c2acb63cf2ef80e1009bdc030c8bafce91bfe/R/Renderer.R#L95-L105

Line 95: we call a method and store returned file path Line 96-104: we add more args to the list Line 105: we call render() on that newly created file which essentially means that we call a code inputted there.

chlebowa commented 11 months ago

My mistake, I missed the fact that the report is rendered when it is saved.

Would you rather have

format_code_block_function <- quote(
    code_block <- function(code_text) {
        df <- data.frame(code_text)
        ft <- flextable::flextable(df)
        ft <- flextable::delete_part(ft, part = "header")
        ft <- flextable::autofit(ft, add_h = 0)
        ft <- flextable::fontsize(ft, size = 7, part = "body")
        ft <- flextable::bg(x = ft, bg = "lightgrey")
        ft <- flextable::border_outer(ft)
        if (flextable::flextable_dim(ft)$widths > 8) {
            ft <- flextable::width(ft, width = 8)
        }
        ft
    }
)

and then

paste(deparse(format_code_block_function), collapse = "\n")

?

pawelru commented 11 months ago

Yes. Way way better than code code as a string.