Conversation
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
air
[air] reported by reviewdog 🐶
Line 5 in 21ccc48
[air] reported by reviewdog 🐶
Line 188 in 21ccc48
[air] reported by reviewdog 🐶
Line 2 in 21ccc48
[air] reported by reviewdog 🐶
Line 5 in 21ccc48
[air] reported by reviewdog 🐶
Line 52 in 21ccc48
[air] reported by reviewdog 🐶
Line 54 in 21ccc48
[air] reported by reviewdog 🐶
Line 5 in 21ccc48
[air] reported by reviewdog 🐶
Line 5 in 21ccc48
[air] reported by reviewdog 🐶
Line 7 in 21ccc48
[air] reported by reviewdog 🐶
Line 76 in 21ccc48
[air] reported by reviewdog 🐶
Line 137 in 21ccc48
[air] reported by reviewdog 🐶
Line 5 in 21ccc48
R/deployTFModel.R
Outdated
| #' Deploy a TensorFlow saved model | ||
| #' | ||
| #' @description | ||
| #' Supported servers: Posit Connect, ShinyApps, and RPubs servers |
There was a problem hiding this comment.
RPubs only supports R Markdown documents; it definitely does not support TensotFlow.
There was a problem hiding this comment.
Thank you for catching this, I will fix it in multiple places.
R/accounts.R
Outdated
| #' Account Management Functions | ||
| #' | ||
| #' @description | ||
| #' Supported servers: All servers |
There was a problem hiding this comment.
According to https://roxygen2.r-lib.org/articles/rd.html#the-introduction, the description needs to briefly describe what the function does. Information about server support feels like it belongs in the details, and perhaps at the end of the function details.
@karawoo - Do you have a different suggestion?
There was a problem hiding this comment.
@aronatkins I purposely placed it on top of the description so that it would be visible right away to anyone trying to use the function. I'm willing to bring it down to the details if that is preferable by all.
There was a problem hiding this comment.
@rodriin - I understand the motivation.
I'm trying to keep us roughly following the R documentation guidance, where the description is supposed to describe what the function does using only one paragraph / a few lines.
Reading this guidance makes me feel like server information may want to describe server compatibility in the description or possibly as a note.
https://cran.r-project.org/doc/manuals/R-exts.html#Documenting-functions
There was a problem hiding this comment.
I would definitely expect the first paragraph of the description section to describe what the function does. I'd be ok with keeping the info on server compatibility in that section, but as a second paragraph after the description of the function.
There was a problem hiding this comment.
Ok, I'll go with adding the server compatibility as the 2nd or last thing on the description section.
| #' Add authorized user to application | ||
| #' | ||
| #' @description | ||
| #' Supported servers: ShinyApps servers | ||
| #' | ||
| #' Add authorized user to application |
There was a problem hiding this comment.
Here and elsewhere, can we update so the title and one paragraph of the description don't exactly duplicate one another?
There was a problem hiding this comment.
@karawoo The only reason I had to duplicate the description from the title in these cases is because the existing documentation did not contain anything for the description hence the generated docs came out with those things duplicated. I'm not sure I have all the knowledge to populate the specific missing descriptions. Maybe that can be done on a separate PR by someone more familiar with the existing functionality?
There was a problem hiding this comment.
Also if I didn't do the duplication for all the cases missing the description, then only the supported server info would be in the description area which didn't feel right since I was also trying to have the generated docs match the existing docs and only have the extra info I was adding for the supported servers.
|
@aronatkins @karawoo This is ready for re-review. Thank you! |
aronatkins
left a comment
There was a problem hiding this comment.
One misplaced update, but otherwise looks good. Thanks for this exhaustive update!
R/accounts.R
Outdated
| #' Register account on Posit Connect | ||
| # | ||
| #' @description | ||
| #' Supported servers: Posit Connect servers |
There was a problem hiding this comment.
This is still the leading text for "Description".
The supported servers should move to the end of the description section.
Screenshot taken from locally installed documentation for ?rsconnect::connectUser
There was a problem hiding this comment.
Thank you for catching this one, I missed it. I'll fix it now.
|
|
||
| #' List invited users for an application | ||
| #' | ||
| #' @description |
There was a problem hiding this comment.
I'm OK with this duplication. It mirrors the implied duplication we had previously. It does point out where we lack sufficient documentation, but you're not addressing that, here.
(screenshot from https://rstudio.github.io/rsconnect/reference/showInvited.html)
Resolves: #1227
This PR adds the supported server info to the documentation for all exposed functions at the very top of the description area. In the cases when the function specifically supports only one type of server it will say the server type. For functions that are supported for all servers it will say: "All servers".