openpharma / roxytypes

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

Documenting format syntax, improving robustness, documenting custom format fns #11

Closed dgkf closed 1 year ago

dgkf commented 1 year ago

Closes #9, tagging @danielinteractive for interest (edit: docs now published here)

Adds proposal in #9. However, this does not provide a helper function. I think this is a reasonable compromise. Providing helper functions for splitting strings on commas is inherently package-specific since commas can be valid class names and might otherwise be expected to work out-of-the-box.

Included in the documentation under ?tags and ?typed, there is an example for multi-class formatting.

Now, by using a custom function one loses the built-in syntax handling. For example, naively wrapping in backticks would render [package::fn()] as a inline code instead of relying on roxygen2 to manage the reference. It's for this reason that I would still recommend using a plain literal, even for multiclass definitions:

#' @typed arg: "`character(1L)` or [special_class]"
codecov[bot] commented 1 year ago

Codecov Report

Merging #11 (1b988fe) into main (f95be2e) will increase coverage by 0.10%. The diff coverage is 0.00%.

:exclamation: Current head 1b988fe differs from pull request most recent head 5cb4ffe. Consider uploading reports for the commit 5cb4ffe to get more accurate results

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
+ Coverage   82.89%   83.00%   +0.10%     
==========================================
  Files          11       11              
  Lines         345      353       +8     
==========================================
+ Hits          286      293       +7     
- Misses         59       60       +1     
Impacted Files Coverage Δ
R/tag_typed.R 58.82% <0.00%> (+18.19%) :arrow_up:
R/tag_typedreturn.R 0.00% <ø> (ø)
R/utils.R 70.37% <0.00%> (-20.11%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

danielinteractive commented 1 year ago

Thanks @dgkf for this! So I tried to use this now in Genentech/jmpost@d5b8f34 (#174) However I am observing that e.g. image is giving the output: image which is not what I would like, the output should be still: image what I can I do better here wrt using roxytypes?

dgkf commented 1 year ago

Very interesting. I'd chalk this up to a bug - or at least an unintended behavior. I added a test for the interpreted string, but not the styled roxygen output, and this is a bit surprising.

I think the issue is with how roxygen interprets the "`code`" style. It seems to be interpreting "` .. `" as a special delimiter? I didn't anticipate that! I'll investigate and hopefully have a fix up soon.

dgkf commented 1 year ago

After digging into this more, I realized it was because the format is being provided by the Config in DESCRIPTION (which masks the default format and therefore won't leverage any of the default type syntax handling).

In that sense, this is "working as intended", but I'm not satisfied with how it was an all-or-nothing choice between benefiting from the default syntax and doing something custom.

I pushed an update today which now applies a default format for each field (type, description) instead of holistically for formatting the whole tag. Internally, each field gets its own S3 class and given a as.character method so that glue can apply the formatting for us. This means that your custom format will now pick up the default handling to preserve literals.

However, part of this change means that the default type formatting will also continue to insert backticks in the "normal" case, so @danielinteractive it should be good if you use the latest version and update your DESCRIPTION with:

- Config/roxytypes: list(format = "(`{type}`)\\cr {description}")
+ Config/roxytypes: list(format = "({type})\\cr {description}")

I tested it on jmpost and this issue looks cleared up. Let me know if you see anything else that isn't as straightforward as you'd like.

danielinteractive commented 1 year ago

Super, thanks @dgkf !

danielinteractive commented 1 year ago

Just tested, and can confirm this solves the issue. thanks again!