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

Use @param when documenting arguments to R6 methods #408

Merged
merged 16 commits into from
Dec 17, 2020
Merged

Conversation

jgabry
Copy link
Member

@jgabry jgabry commented Dec 16, 2020

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

Closes #367.

Uses @param in empty docstrings (no function) when documenting CmdStanModel and CmdStanFit methods. This will allow other developers like @wlandau to use roxygen2's @inheritParams to import our doc into packages that use cmdstanr. This PR is unrelated to #81 and doesn't use roxygen2's limited R6 support.

I think this is a good idea but there are some downsides to doing this that we should also consider:

  • For regular functions in an R package (e.g. functions like write_stan_json() or cmdstan_model()) the Usage section in the doc (showing the function signature) comes before the Arguments section. This is done automatically by roxygen2. However, when using empty docstrings with @param to document the R6 methods the Arguments section is automatically generated but we still have to manually generate the Usage section. This results in the Usage section coming after the Arguments section. I really don't like this but I don't know if it's a dealbreaker. (I'm also not sure if there's a way around the ordering that roxygen2 is generating. Maybe we can modify it.)

  • As far as I know, we can't have subsections in the Arguments section in the doc. When using @param the Arguments section is automatically generated. Previously we were breaking it up into subsections to separate args shared by all model fitting methods and method specific args (and in the latter we were splitting again into args in CmdStanR but not CmdStan vs args in both CmdStanR and CmdStan). When using @param it all becomes one long list of arguments with no breaking it up with subsection descriptions.

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Columbia University

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@jgabry
Copy link
Member Author

jgabry commented Dec 16, 2020

@rok-cesnovar I'll generate the website docs once we're ready to merge this. Or if you want to review what that looks like I can also generate them now so you can just use pkgdown::preview_site() to view the changes.

man-roxygen/model-sample-args.R Outdated Show resolved Hide resolved
man-roxygen/model-sample-args.R Outdated Show resolved Hide resolved
R/model.R Show resolved Hide resolved
R/model.R Outdated Show resolved Hide resolved
@jgabry jgabry changed the title Use @params when documenting arguments to R6 methods Use @param when documenting arguments to R6 methods Dec 16, 2020
@codecov-io
Copy link

codecov-io commented Dec 16, 2020

Codecov Report

Merging #408 (58b5b0c) into master (dcdffb7) will decrease coverage by 0.12%.
The diff coverage is 94.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #408      +/-   ##
==========================================
- Coverage   88.30%   88.18%   -0.13%     
==========================================
  Files          12       12              
  Lines        2762     2767       +5     
==========================================
+ Hits         2439     2440       +1     
- Misses        323      327       +4     
Impacted Files Coverage Δ
R/knitr.R 91.66% <ø> (ø)
R/model.R 80.33% <ø> (ø)
R/fit.R 96.29% <94.62%> (-1.61%) ⬇️
R/args.R 99.56% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcdffb7...58b5b0c. Read the comment docs.

wlandau pushed a commit to ropensci/stantargets that referenced this pull request Dec 16, 2020
@wlandau
Copy link
Collaborator

wlandau commented Dec 16, 2020

Thank you SO much for this, @jgabry! It will be a whole lot easier to keep up with changes in cmdstanr for stantargets. I am using your PR in this stantargets branch, and R CMD check seems totally satisfied with the docs.

@jgabry
Copy link
Member Author

jgabry commented Dec 16, 2020

Thank you SO much for this, @jgabry! It will be a whole lot easier to keep up with changes in cmdstanr for stantargets. I am using your PR in this stantargets branch, and R CMD check seems totally satisfied with the docs.

Awesome! And yeah R CMD check seems to be fine. At this point the only thing I'm worried about is this:

For regular functions in an R package (e.g. functions like write_stan_json() or cmdstan_model()) the Usage section in the doc (showing the function signature) comes before the Arguments section. This is done automatically by roxygen2. However, when using empty docstrings with @param to document the R6 methods the Arguments section is automatically generated but we still have to manually generate the Usage section. This results in the Usage section coming after the Arguments section. I really don't like this but I don't know if it's a dealbreaker. (I'm also not sure if there's a way around the ordering that roxygen2 is generating. Maybe we can modify it.)

Even if I put the manually created Usage section before the @param entries roxygen2 will put the automatically generated Arguments section before the manually created Usage section. I suppose we could write some function to parse the generated Rd files and change the order of the sections after roxygen2 generates them, but that could be error prone and doesn't seem like a great solution. I know there are ways to extend roxygen2 but I haven't figured out yet if that would allow something like changing the order of the sections.

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks!

@jgabry
Copy link
Member Author

jgabry commented Dec 16, 2020

Actually, I have an idea about how to deal with the usage sections. Not sure if it will work but I will try now and if it does I'll explain.

R/model.R Show resolved Hide resolved
@jgabry
Copy link
Member Author

jgabry commented Dec 16, 2020

Ok I was able to fix the Usage section problem the way I mentioned for the compile method above. I think this is now ready.

@wlandau Can you double check that this still works well for you after the changes I made to fix the Usage section issue? I don't think it should change anything for you but it would be good to know if it still works and if R cmd check still is fine for you. Thank you!

@wlandau
Copy link
Collaborator

wlandau commented Dec 16, 2020

Just ran R CMD check again on 8810cd7 and found no issues with the docs.

@jgabry
Copy link
Member Author

jgabry commented Dec 17, 2020

Thanks! R cmd check runs fine for me too but it's good to know that you're seeing the same thing. Thanks again for suggesting this.

@jgabry
Copy link
Member Author

jgabry commented Dec 17, 2020

@rok-cesnovar Are you ok if I make this (and everything else that's been merged recently) into a release? If so do you prefer 0.2.3 or 0.3.0?

@jgabry jgabry merged commit 46ae80f into master Dec 17, 2020
@jgabry jgabry deleted the use-params-for-doc branch December 17, 2020 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formal @param entries for method arguments
4 participants