openpharma / roxytypes

typed parameter definitions for `roxygen2`
https://openpharma.github.io/roxytypes/
Other
4 stars 1 forks source link

Discussion: Reconsider `@default` tag #5

Open dgkf opened 1 year ago

dgkf commented 1 year ago

This tag is "fake", it's processed within the @typed tag.

Right now the roxytypes package also has a feature that lets you generate a little snippet that describes a parameter's default value. roxytypes has the ability to parse this default value from the package, or use a provided default description using the @default value.

I think this is a cool capability, but it probably shouldn't be coupled to the @typed tag. You could just as readily want to document a default value for a regular @param, eg

#' @param var This is a description. @default FALSE

to generate something like

Arguments: 

    `var`: This is a description. (Defaults to `FALSE`)
danielinteractive commented 1 year ago

My 2 cents on this default behavior: I would not use this. Because: 1) default argument value as code can be read of the usage in the documentation already. So this would be duplicate. 2) If any explanation of the default argument value is useful, then in human form, which cannot be parsed automatically. (Well at least as of now :-D)

dgkf commented 1 year ago

Yeah, I agree with you on both accounts, but I still see this pattern being used quite a lot. Even if the value is minor, I think there is some merit to it:

  1. It supports IDEs which use the argument definition as a tooltip for completions.
  2. It can help to clarify how defaults run through match.arg are used (which treats the formals as different from the default).
  3. It allows for documentation about default behaviors when the behavior is different from the value. You'll occasionally see functions that might have a default behavior equivalent to a value (e.g. FALSE) even if the default is NULL or missing.

Even if there's value here, I think it's clear that whatever value it might offer would be outside the needs of a documenting types. I'm in favor of shuffling this into another little pilot package and we can revisit it at a different time.

danielinteractive commented 1 year ago

Ah now I get it - yes as a manually specified tag this makes a lot of sense. Also fits in here. I would just not try to have the package infer the default automatically from the function definition, for above mentioned reasons.

I think for automatic population of anything in the roxygen chunk, might fit better in the https://github.com/yonicd/sinew package. Especially for the default value, it could be nice to prepopulate and then manually adjust (or delete).

dgkf commented 1 year ago

I would just not try to have the package infer the default automatically from the function definition, for above mentioned reasons.

Yep - that's disabled by default.

I think for automatic population of anything in the roxygen chunk..

The only reason I think it fits better as a roclet is that you can add checks for when defaults are undocumented. I think sinew already supports having a default as part of its template.

All things told, let's put this feature on hold then. Appreciate the feedback!

danielinteractive commented 1 year ago

Thanks Doug - I think the minimal version without automated generation is still very useful and would be nice to have

dgkf commented 1 year ago

Documenting decision:

I'm going to strip this feature out for now since I don't want to commit to supporting it without a bit of a better handle on how I want it to work or whether it should live alongside type definitions.

It will forever be part of the commit history, but for visibility I'm going to create a branch to house this experiment before stripping out the code.