insightsengineering / rtables

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

rename function "as_ard" to "as_dataframe_with_spec" #670

Closed shajoezhu closed 1 year ago

shajoezhu commented 1 year ago

hey guys @gmbecker @Nolan-Steed , I would like to suggest renaming this function “as_ard", given ARD is a much larger topic and yet to be defined. I think this is a very great first step, but might be misleading later on. I would suggest to rename this function for now. Thanks!

cc @telepath37 @pawelru @khatril for awareness

gmbecker commented 1 year ago

The whole point of engineering it the way I did is that regardless of what specification we eventually arrive at for ARDs, that will just be a spec that we implement here, no need to change the function. It is future proofed @shajoezhu

shajoezhu commented 1 year ago

Hi @gmbecker , the worry is that ARD itself hasn't been defined properly. i think it is going to be a much more complex structure than a dataframe. please allow us to have some time to refine the ARD issue before we name it in such way. thanks

nsteed15 commented 1 year ago

If this function gets exported in the next release, it might be a safer option to rename it so end users do not get confused with the ARD end product. Those are my thoughts.

gmbecker commented 1 year ago

How about one of the following:

  1. as_ard_df - this distinguishes "an ARD" from the data.frame containing ard information
  2. as_result_df
  3. extract_value_df
  4. extract_result_df
  5. values_df

Or something similar.

@shajoezhu @Nolan-Steed @Melkiades @edelarua @ayogasekaram

shajoezhu commented 1 year ago

Thanks Gabe. I would vote for

as_result_df or extract_result_df

Melkiades commented 1 year ago

Me too, 2 and 4

pawelru commented 1 year ago

3, 5

nsteed15 commented 1 year ago

I vote for 2

shajoezhu commented 1 year ago

I think we have a winner. let's do 2.

@pawelru , we had discussed the option again, the outcome of this function doesn't just include values, but also the grouping information. we think "results" is more appropriate than "values" in this case. :)