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

More granular documentation #43

Closed
AdamSpannbauer opened this issue May 6, 2019 · 5 comments

Comments

Projects
None yet
2 participants
@AdamSpannbauer
Copy link
Contributor

commented May 6, 2019

This is essentially a re-opening of #6 (closed due to inactivity). I'm using this package on a project and would like to include more documentation around RcppAnnoy to coworkers.

I think it would make the most sense for this to be included in the package, but I will document locally if the answer to the following question is "No". Would a PR using roxygen to document the Annoy* family be welcome?

I was thinking of using a similar style as what's used here to document an R6 class and its methods. Using roxygen adds RoxygenNote: 6.1.1 & Encoding: UTF-8 to DESCRIPTION.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented May 6, 2019

Thanks for proposing to add documentation :)

I think the last documentation was "just" a rather vignette (about the somewhat even more specialised case of C++ use RcppAnnoy headers). So we could also do a vignette about R use.

I had, as you may have seen (in their repo under an unsolved but of course "closed" issue as that is how they rol), bad luck with more recent roxygen2 version freezing my use at version 6.0.1. But apart from that we could go that route too. (And I have not been bitten yet by others using 6.1.1; it just notes the different versions and whines about it.)

One "minor" irritant is that thanks to templates we have the same code a few times. So we want to make we write to docs just once. A block ending in NULL may be one way.

@AdamSpannbauer

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I've added a first pass at the documentation in my fork's annoy.R.

I documented on NULL as you allude to in your comment. I additionally, ported the package level documentation to roxygen2 style and placed it at the top of annoy.R. If this isn't wanted I can delete.


I wanted to get some clarification on your note before opening up a PR for review.

I think the last documentation was "just" a rather vignette (about the somewhat even more specialised case of C++ use RcppAnnoy headers). So we could also do a vignette about R use.

Was this indicating that you'd like me to take a stab at writing the vignette in my PR or rather that you'd like to include a vignette (to be written by you) in the same release as other documentation?

I had, as you may have seen (in their repo under an unsolved but of course "closed" issue as that is how they rol), bad luck with more recent roxygen2 version freezing my use at version 6.0.1. But apart from that we could go that route too. (And I have not been bitten yet by others using 6.1.1; it just notes the different versions and whines about it.)

I interpreted this as "the changes roxygen2 makes to DESCRIPTION are not needed". Was this the correct interpretation? If not I can push an updated DESCRIPTION.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented May 6, 2019

Will peruse your fork.

Re vignette: there was a "specialed" missing in front of vignette. The vigette is cool, so we could have gone that way. Direct docs are good too.

Re roxygen2: I am fundamentally unhappy about upstream broke a perfectly valid usage patter simply because it didn't fit their mold. I am surviving but it other innocent bystanders (as eg students of mine at deadline of last homework ... ).

@AdamSpannbauer

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I'll go ahead and open a PR to make review easier

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented May 10, 2019

Thanks for closing the ticket, and the documentation. The tinytest package I was waiting on is now on CRAN so this should go out this weekend too.

eddelbuettel added a commit that referenced this issue May 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.