Documentation: Clearer documentation needed for functions that require exprs() to be used in input to arguments

[chelseadickens]

Please select a category the issue is focused on?

Function Documentation

Let us know where something needs a refresh or put your idea here!

I would suggest adding more details about the expected inputs to parameters that require using exprs(). For example, the help documentation for derive_var_merged_exist_flag() makes no mention of needing to use exprs() when specifying by_vars:

A user would need to either track back to the documentation for derive_vars_merged() to see more detail about the expected input or assume from the examples at the end of the documentation for derive_var_merged_exist_flag() that they need to use exprs().

I would suggest incorporating some of the details from derive_vars_merged() into the documentation of other functions that have similar requirements:

[bundfussr]

@chelseadickens , thanks for creating this issue. I think it is a good idea to unify the descriptions.

@pharmaverse/admiral , looks like a good use case for our new roxygen functions concept. Should we create a roxygen_permitted_exprs() function?

[bms63]

Yes great use case for roxygen function!!

@chelseadickens would you like to make the updates - we can assist you if needed. admiral team likes to get users involved in updates as we are users ourselves!!

[manciniedoardo]

really like this idea! thanks for sharing @chelseadickens !

[tgerke]

Thanks @chelseadickens for suggesting this (I just landed on this request when I was going to submit a very similar issue) and thanks admiral team for all you do!

From a user perspective, I also find the requirement for arguments wrapped inexprs confusing/non-standard. Once I dug into the code base and stumbled on the help for derive_vars_merged, I see it now. That said, I can picture long-term challenges needing to explain the advanced concepts of defusing and exprs to team members when they pick up admiral for the first time.

I do appreciate that it looks like the notion of accepting tidyselect arguments was discussed recently, and it looks like it may be tackled in the future (which would be awesome!). If that is the plan though, just dropping a quick thought that it may be wise to weigh (understandably limited!) bandwidth for that change against writing lots of further documentation around exprs. The linked issue thread noted stability concerns around tidyselect, but I would think current version 1.2.0 is comparably stable from the user perspective to rlang version 1.1.1, upon which the use of exprs depends.

[manciniedoardo]

Thanks for the comment @tgerke. We did weigh up the option of implementing this change before the upcoming admiral 1.0 release, but we cannot commit to having enough time for this big undertaking, and also don't feel it would be right to overhaul the package right before 1.0. As you have noted though we have earmarked this suggestion for future work once we have achieved a stable 1.0 release.

[tgerke]

Thanks for the response @manciniedoardo! Understood, will look forward to stable 1.0 (exciting stuff!) and the future improvements.

[bms63]

Hey @StefanThoma will you be able to complete this before our release on December 4th? I think this will be really helpful for users wondering about exprs and helped bridge the gap from 1.0 and 2.0

[manciniedoardo]

@StefanThoma Were you thinking of implementing as a vignette or something else?

[StefanThoma]

@StefanThoma Were you thinking of implementing as a vignette or something else?

@manciniedoardo I was just planning to unify the description of by_vars using a function.
Do you think a vignette is necessary? The examples for each function should also help to guide users.

[manciniedoardo]

@manciniedoardo I was just planning to unify the description of by_vars using a function. Do you think a vignette is necessary? The examples for each function should also help to guide users.

I think the unification is a good idea, moreover if it could go hand-in-hand with a (short) vignette explaining the exprs() paradigm I think that could be a useful landing page/redirect for users with questions on the subject. @pharmaverse/admiral thoughts?

[bundfussr]

@manciniedoardo I was just planning to unify the description of by_vars using a function. Do you think a vignette is necessary? The examples for each function should also help to guide users.

I think the unification is a good idea, moreover if it could go hand-in-hand with a (short) vignette explaining the exprs() paradigm I think that could be a useful landing page/redirect for users with questions on the subject. @pharmaverse/admiral thoughts?

I wonder if we need a separate vignette. Maybe we could just link to https://pharmaverse.github.io/admiral/dev/articles/admiral.html#arguments (and update/extend this section if necessary).

[bms63]

@manciniedoardo I was just planning to unify the description of by_vars using a function. Do you think a vignette is necessary? The examples for each function should also help to guide users.

I think the unification is a good idea, moreover if it could go hand-in-hand with a (short) vignette explaining the exprs() paradigm I think that could be a useful landing page/redirect for users with questions on the subject. @pharmaverse/admiral thoughts?

I wonder if we need a separate vignette. Maybe we could just link to https://pharmaverse.github.io/admiral/dev/articles/admiral.html#arguments (and update/extend this section if necessary).

I think updating and linking to an existing vignette is the way to go.

[manciniedoardo]

Sounds good to me too, thanks for the inputs @bms63 @bundfussr

[StefanThoma]

Does the by_vars argument serve the same purpose in all functions where present?
In derive_adeg_params the by_vars argument is described as:

Only variables specified in by_vars will be populated
in the newly created records.

[StefanThoma]

I would suggest to keep the function specific content and simply add a general description of how to use the argument underneith. What do you think @bms63 @manciniedoardo ?

[bundfussr]

I would suggest to keep the function specific content and simply add a general description of how to use the argument underneith. What do you think @bms63 @manciniedoardo ?

Makes sense to me. I think it is just the "Permitted Values:" part which should be unified.

[bms63]

I would suggest to keep the function specific content and simply add a general description of how to use the argument underneith. What do you think @bms63 @manciniedoardo ?

It sounds good to me as well!