insightsengineering / rtables

Reporting tables with R
https://insightsengineering.github.io/rtables/
Other
227 stars 49 forks source link

Migrate Exporter logic to `formatters` #491

Open gmbecker opened 1 year ago

gmbecker commented 1 year ago
shajoezhu commented 1 year ago

Hi @gmbecker , I think this is really good to move out of rtables. Though I wonder if formatters is the best place to host these export functionalities. from a workflow point of view, these should be downstream functionalities. something like export_rtables_to_pdf, export_rlistings_to_pdf, export_rtables_to_rtf, export_rlistings_to_rtf, I wonder we should start a new package for the downstream process

gmbecker commented 1 year ago

@shajoezhu, whole point is that we would not have export_rtables_bla and export_rlistings_bla, we only need one because it is a function of the MatrixPrintForm (and its embedded pagination_dfs). That is why it belongs in formatters, because it is shared functionality.

shajoezhu commented 1 year ago

absolutely agree: we just need one

my point is from package dependency point of view. rtable creates an rtable object, rlisting creates a listing object. exporting is for something downstream, which can render to rtf, or pdf at the moment, in the future, we may also want ppt, flexitable, latex code. these exporting utilities should be used after rtable or listing object are created. placing these machinaries upstream is a little counterintuitive. and MatrixPrintForm happens downstream of rtables and listing as well. do you think it would make sense to take MatrixPrintForm out of formatters, and creating another package downstream instead?