Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation for R6 methods added dynamically by "$set()" #931

Open
coolbutuseless opened this issue Oct 5, 2019 · 19 comments
Open

Documentation for R6 methods added dynamically by "$set()" #931

coolbutuseless opened this issue Oct 5, 2019 · 19 comments
Labels
feature a feature request or enhancement R6 6️⃣

Comments

@coolbutuseless
Copy link

coolbutuseless commented Oct 5, 2019

I've just converted minihtml over to latest roxygen2 from github to test out the R6 support and it's working pretty well.

One challenge at the moment is that I generate many R6 methods for HTMLElement class dynamically using HTMLElement$set("public", method_name, method). I can not see any way to generate documentation for these methods and roxygen is very noisy in telling me that neither the methods nor any of their arguments are documented!

Click to see section of warnings generated during documentation step
(... snipped a 100 lines at the start ...)
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `title()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `tr()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `track()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `tt()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `ul()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `var()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `video()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] `wbr()`: undocumented R6 method
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] argument `...` undocumented for R6 method `a()`
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] argument `href` undocumented for R6 method `a()`
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] argument `hreflang` undocumented for R6 method `a()`
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] argument `media` undocumented for R6 method `a()`
Warning: [/Users/mike/projects/minihtml/R/HTMLElement.R:1] argument `rel` undocumented for R6 method `a()`

(... snipped a few hundred lines at the end ...)

Questions:

  • Is there a way to document methods added dynamically with a $set()?
  • Is there a way to tell roxygen2 to be quieter about missing documentation for methods?
  • Is there a way to tell roxygen2 that although some methods are public I do not want to include them in the documentation?

Sidenote: Whether or not it's a good idea to generate a method for each of the ~100 HTML tags is a different conversation :)

@gaborcsardi
Copy link
Member

Is there a way to document methods added dynamically with a $set()?

Nope.

Is there a way to tell roxygen2 to be quieter about missing documentation for methods?

Nope, except for suppressWarnings() which you probably don't want.

Is there a way to tell roxygen2 that although some methods are public I do not want to include them in the documentation?

Nope.

All of these would be reasonable features, especially the first and the third. The third one seems easy, we could allow @keyword internal in the documentation of the method, and then these would be omitted. This does not really help you if the method is created with set().

The current scheme just assumed that all docs are in a single roxygen block, and the methods docs are inline.

@gaborcsardi
Copy link
Member

gaborcsardi commented Oct 5, 2019

@mikefc Please open issues for $set() and non-documented methods.

Edit: I guess this one is for $set(). :)

@gaborcsardi
Copy link
Member

Looking at your use of $set() at https://github.com/coolbutuseless/minihtml/blob/46f1cf6c9c3243fff4fb50948161625a817fc5d8/R/htag.R#L264
I guess you don't want to add documentation for each method....

@coolbutuseless
Copy link
Author

For this case I don't really want to add documentation for any of those methods. At the most I'd want to somehow lump them into a single documentation element which would just point the user to external documentation on the web.

@gaborcsardi
Copy link
Member

gaborcsardi commented Oct 5, 2019 via email

@gaborcsardi
Copy link
Member

Re $set() support, I can already see some challenges.

One is that currently the R6 doc generator works at the block level, and it should really generate the output later at the topic level, so we can use data from multiple blocks. This is an implementation detail, and we can change it, it is mostly just refactoring.

The second is that I am not sure how we would detect $set() calls. They just return NULL AFAICT, so in

#' @description Foo.
#' @param bar Bar.
myclass$set('public', method_name, method_fun) 

roxygen needs to be able to tell that this call sets a method on an R6 class.

Another thing is that if a method is not added by the time the package env is loaded, but only later, then we'll not see it. I guess there is not much we can do about this.

@hadley
Copy link
Member

hadley commented Oct 6, 2019

I don’t think $set() returning null should be a problem - you can just inspect components of the call to find the right class.

@gaborcsardi
Copy link
Member

I don’t think $set() returning null should be a problem - you can just inspect components of the call to find the right class.

For the simple cases, yes. But $set() does allow some very funky stuff as well. E.g.

.onLoad <- function(libname, pkgname) {
  #' @param x, y params
  myfun <- function(x, y) { }
  myclass$set('public', 'mymethod', myfun)
}

which is probably not something we want to support. I guess the challenge is to investigate the forms we do want to support.

Anyway, I don't think $set() support is that urgent, the OP does not actually want to document all the 100+ dynamically added methods.

@gaborcsardi
Copy link
Member

FYI, this will not make it into the next roxygen release, but we still want to implement it later.

@hadley hadley added feature a feature request or enhancement R6 6️⃣ labels Nov 5, 2019
@jgabry
Copy link

jgabry commented Nov 13, 2019

Glad you’re planning on implementing this! Will be super helpful for the new stan-dev/cmdstanr which we’ll release with r6=false or converting every method to being defined with the class so we can use inline doc, but looking forwards to this.

And thanks for the R6 functionality you’ve already inciuded!

@hongooi73
Copy link

Something else to consider as well: it's possible for a package to use set to add new members to a class defined in a different package. I use this a lot in my AzureR packages, to extend a small set of base classes to support various Azure services.

This was previously mentioned in #807 but prematurely closed.

@hongooi73
Copy link

Also, I think supporting set is an important feature to have for another reason: with complex R6 classes, it's often a maintainability plus to split the class over multiple source files. Without set, you'd have to implement the class as a single giant block of code. But this falls down if Roxygen doesn't support it.

@dvg-p4
Copy link

dvg-p4 commented Jan 19, 2023

Any current plans to add support for this?

@dvg-p4
Copy link

dvg-p4 commented Jan 19, 2023

For our use case, it would also be helpful to have the class' methods documented in separate files--there are a lot of them, hence our pattern of splitting them into multiple R files using $set in the first place

@dvg-p4
Copy link

dvg-p4 commented Jan 19, 2023

At the very least, it would be nice not to be spammed with warnings for not properly documenting methods that roxygen provides no way to properly document.

@gaborcsardi
Copy link
Member

You can set r6 = FALSE, which lets you organize the classes, methods and files any way you like.

@dvg-p4
Copy link

dvg-p4 commented Apr 10, 2023

You can set r6 = FALSE, which lets you organize the classes, methods and files any way you like.

How should we format the documentation then?

@gaborcsardi
Copy link
Member

Here is an example that we use before roxygen2 had any R6 support: https://github.com/r-lib/processx/blob/v3.4.2/R/process.R#L5

@dvg-p4
Copy link

dvg-p4 commented Apr 11, 2023

Ah I see, just document NULL and write out all the tags manually. Yeah that's about what we've been doing, was just wondering if there was a better way--guess not at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement R6 6️⃣
Projects
None yet
Development

No branches or pull requests

6 participants