Ensure consistency in roxygen argument selection, documentation and order in function headers

[sgorm123]

Please select a category the issue is focused on?

Function Documentation

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

Splitting out Discussion issue #80

Ensure consistency in roxygen argument selection, documentation of arguments (e.g. permitted values, required or optional, examples) and order in function headers

Awaiting @thomas-neitmann draft guidance on the above.

[thomas-neitmann]

Sorry for being a bit late.

Some first thoughts:

• No need to add "Required" or "Optional" in parameter documentation. By our programming strategy pptional parameters are those that have NULL as default, everything else is required and those parameter that don't have a default value have to be explicitly set by users.
• Only use "Permitted values:" if there is a limited number of strings to choose from (think a CDISC code list), e.g. for the mode parameter we often use only "first" and "last" are valid so these values should be listed along with the default. Don't say something like "Permitted values: data frames".
• The description of every parameter should indicate the expected data type like data.frame, character or logical.

[bundfussr]

I would prefer using "Permitted values" for all parameters. Then this information is easier to find. At the moment we are not using "Permitted Values" consistently. I think it would be good to be consistent.

[sgorm123]

Thanks @thomas-neitmann.

Ross @rossfarrugia

1. "Required" or "Optional": shall I remove these from the bor function? I like this, but it is not needed in all, so maybe better to be consistent.
2. Permitted values: Who makes this decision? I agree with @bundfussr, but defer to others final judgement.

Also, what about the Default: argument, I presume this is this only necessary when there is a default set in the function signature, even NULL? I can update this issues as part of: https://github.com/pharmaverse/admiralonco/tree/128_filter_pd_optional_assertions_updated%40devel

Once confirmed.

Thank you all

[rossfarrugia]

@sgorm123

1. yes please remove these
2. i added to core dev team meeting agenda for today to push for a decision and clarity - will also check on your defaults question

[thomas-neitmann]

I don't think having a Default is particularly useful. You already see that on top in the function signature.

[sgorm123]

@rossfarrugia so shall I remove all default text, they are currently in the majority of admiralonco functions?

This is also common in the admiral functions, e.g. https://github.com/pharmaverse/admiral/blob/main/R/derive_param_first_event.R#:~:text=%23%27%20%20%20*Default*%3A%20%60%22warning%22%60

[sgorm123]

One plus side of having defaults is I can see them in the inline help, without having to scroll up to the signature (see max_nr_ne below):

[rossfarrugia]

Summary of discussions at core dev meeting (FYI @sgorm123)

• Defaults: Agreed with Thomas to remove all these for now as its a repeat of info from Usage section and likely might get out of sync/hard to maintain, but a nice to have might be a way to automate this later - as we did see value in not having to scroll up to see this as per Stephen's point
• Permitted values: We agreed these are useful to include for all arguments as it could help bring clarity of expectations to users, but before we add these we should have some controlled terminology to apply (see Documentation: Clarity on documentation of function arguments admiraldev#64). So don't worry about updating these for admiralonco yet and we can come back to this bit later

Follow-up related issues created for later as FYI:

• Idea for future: Any way to automate adding default argument values in roxygen headers? admiraldev#270
• Documentation: Clarity on documentation of function arguments admiraldev#64
• Documentation: Examples of when to use Default and Permitted Values admiraldev#72
• Documentation: Explore better way to show default arguments where some internal derivation needed admiral#1402

[sgorm123]

@rossfarrugia Ive removed all the defaults, updated to make filter_pd optional, and also removed duplicated assertions.

Im just a little worried about creating a PR until the other items on the devel branch have been accepted, given that there will be conflicts. Should I wait a little to create the PR for issue #128

[rossfarrugia]

@sgorm123 yes, as discussed at call please merge #121 first

[zdz2101]

Closing to centralize as denoted in pharmaverse/admiral#1976