-
Notifications
You must be signed in to change notification settings - Fork 25
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
[r] Docs and NAMESPACE cleanup #1566
Conversation
These are both available as methods on the appropriate R6 classes
This reverts commit e1a7931.
#' @method pad_matrix default | ||
#' @export |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @export
tags are (IIRC) still needed for the method definitions to enable S3 method dispatch (@export
on a function vs @export
on a method are different)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(looking at the diff for NAMESPACE
shows that the method definitions are no longer registered)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have @export on a ton of methods in tiledb-r too. Plus occasional exportClass.
I think roxygen2 behavior changed here over the years.
(Usual disclaimer: roxygen2 is something "we" (as in R users) do, it is not normative or even mentioned really in WRE. Only the final NAMESPACE and man/ matter.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that roxygen seems to do the right thing here and aggregates under the method. One file man/pad_matrix.Rd
generated. For your ease of reading I have:
% Generated by roxygen2: do not edit by hand
% Please edit documentation in R/pad_matrix.R
\name{pad_matrix}
\alias{pad_matrix}
\alias{pad_matrix.matrix}
\title{Pad a Matrix}
\usage{
pad_matrix(x, ...)
\method{pad_matrix}{matrix}(x, rowidx, colidx, shape, sparse = FALSE, ...)
}
\arguments{
\item{x}{A matrix-like object}
\item{...}{Arguments passed to other methods}
\item{rowidx, colidx}{Integer indices for rows and columns from
\code{x} to \code{shape}}
\item{shape}{Target shape to pad out to}
\item{sparse}{Return a \link[Matrix:TsparseMatrix-class]{sparse matrix}}
}
\value{
A matrix containing the data of \code{x} padded out to \code{shape}
}
\description{
Pad a Matrix
}
\keyword{internal}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Disclaimer: It is correct if one wants the method public. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mojaveazure are you suggesting S3 dispatch for pad_matrix() will not work if it's not publicly exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; I've had issues where S3 dispatch fails when the methods are not exported. Happy to test this out more if need be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a quick toy example to test and it doesn't appear to be a problem. Check it out and let me know if I've missed anything: https://github.com/aaronwolen/reverse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using @aaronwolen's example package, it seems as though S3 dispatch works as long as usage is within-namespace. The moment we call the generic from outside of the namespace, we lose dispatch. I think this is acceptable behavior for pad_matrix()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a non-issue? Once we decide to not export it, we pretty much say it won't (or may not) work outside the package -- and we care mostly about the use inside the package. So ... onwards!
Feared loss of functionality proved untrue
@@ -22,9 +22,10 @@ map_query_layout <- function(layout) { | |||
) | |||
} | |||
|
|||
#' Display Package Versions | |||
#' Display package versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have used Title Style on purpose. But we are possibly not quite consistent across so ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that the caveat that it is a long PR -- long good. Thanks for doing that.
The goal of this PR is to reduce the number of exported functions and cleanup our documentation index to make it more user friendly.
I'm very open to feedback on this PR with regard to what should be exported and what should be internal. As per our guidelines for exporting functions I'm unexporting low-level functions that I don't think users would/should use directly. We can always decide to export more classes and functions in the future but it is harder to remove them once they are exported. So let's start with a minimal set of functions.
This gives us more flexibility to change the internal implementation of the package without breaking user-facing APIs.
Internal functions
Functions that are no longer exported:
soma_array_reader
sr_setup
sr_next
sr_complete
pad_matrix
nnz
(this is a method ofSOMASparseNDArray
)shape
(this is a method ofTileDBArray
and its child classes)libtiledbsoma_version()
/tiledb_embedded_version()
(I can definitely see a use case for exporting these so users can programmatically check the versions of libtiledbsoma and tiledb embedded but for now users can get all necessary version information fromshow_package_versions()
)These functions were not exported but were generating Rd files. @mojaveazure pointed out that CRAN requires all functions with an associated Rd file to be exported, so they have been tagged with
@noRd
to prevent them from generating Rd files.soma_array_to_arrow_table
arrow_table_to_dense
arrow_table_to_sparse
Low-level base classes
These classes are all exported and marked as
internal
so they aren't advertised to users. The corresponding Rd files are still generated for developers but they will not appear in the documentation index.Importantly their methods will still appear in the documentation of higher-level classes that inherit from them.
TileDBObject
TileDBArray
TileDBGroup
SOMACOllectionBase
EphemeralCollectionBase
SOMAArrayBase
SOMANDArrayBase
MappingBase
ScalarMap
SOMAContextBase
ReadIter
SOMASparseNDArrayRead
SOMAAxisIndexer
(previously unexported)Ideally we would not export these classes at all but since CRAN requires Rd-generating classes/functions to be exported it would seem our hands are tied.
Other changes:
The following functions are now documented in a single Rd file:
tiledbsoma_stats_disable()
tiledbsoma_stats_dump()
tiledbsoma_stats_enable()
tiledbsoma_stats_reset()
tiledbsoma_stats_show()
Notes for Reviewer: I had also planned to cleanup the pkgdown site as well but I'm going to save that for a subsequent PR.