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

ShortRead imports more than necessary #28

Closed
benjjneb opened this issue Aug 6, 2015 · 4 comments
Closed

ShortRead imports more than necessary #28

benjjneb opened this issue Aug 6, 2015 · 4 comments

Comments

@benjjneb
Copy link
Owner

benjjneb commented Aug 6, 2015

The ShortRead package is importing more than necessary, which adds some time and excess messages during package loading.

Is it possible to import just part of a package? Joey?

@spholmes
Copy link
Collaborator

spholmes commented Aug 6, 2015

You could require that it is installed and then not require(ShortRead) but
just use
ShortRead::function.

On Thu, Aug 6, 2015 at 10:22 AM, benjjneb notifications@github.com wrote:

The ShortRead package is importing more than necessary, which adds some
time and excess messages during package loading.

Is it possible to import just part of a package? Joey?


Reply to this email directly or view it on GitHub
#28.

Susan Holmes
Professor, Statistics and BioX
John Henry Fellow in Undergraduate Education
Sequoia Hall,
390 Serra Mall
Stanford, CA 94305
http://www-stat.stanford.edu/~susan/

@benjjneb
Copy link
Owner Author

benjjneb commented Aug 6, 2015

derepFastq is core functionality though, so shouldn't depend on a require.

On Thu, Aug 6, 2015 at 10:26 AM, spholmes notifications@github.com wrote:

You could require that it is installed and then not require(ShortRead) but
just use
ShortRead::function.

On Thu, Aug 6, 2015 at 10:22 AM, benjjneb notifications@github.com
wrote:

The ShortRead package is importing more than necessary, which adds some
time and excess messages during package loading.

Is it possible to import just part of a package? Joey?


Reply to this email directly or view it on GitHub
#28.

Susan Holmes
Professor, Statistics and BioX
John Henry Fellow in Undergraduate Education
Sequoia Hall,
390 Serra Mall
Stanford, CA 94305
http://www-stat.stanford.edu/~susan/


Reply to this email directly or view it on GitHub
#28 (comment).

@joey711
Copy link
Collaborator

joey711 commented Aug 6, 2015

Yes, you should not do a require. While we were prototyping it didn't make sense to worry too much about pairing down the namespace. Now that we're prepping for BioC submission, we absolutely should think about it.

The best-practice approach is to use the importFrom tag in roxygen:

#' @importFrom ShortRead ShortReadMeth

Instead of loading all of ShortRead, we just specify the exact methods within ShortRead that dada2 will use. It's much cleaner, and avoids some collisions among the packages that dada2 depends on.

@benjjneb
Copy link
Owner Author

benjjneb commented Aug 6, 2015

Where does this roxygen incantation go?

Right now the ShortRead import is happening from the "ShortRead (>=
1.24.0)" being included in the Imports: part of the Description file.

On Thu, Aug 6, 2015 at 12:31 PM, Paul J. McMurdie notifications@github.com
wrote:

Yes, you should not do a require. While we were prototyping it didn't
make sense to worry too much about pairing down the namespace. Now that
we're prepping for BioC submission, we absolutely should think about it.

The best-practice approach is to use the importFrom tag in roxygen:

#' @importFrom ShortRead ShortReadMeth

Instead of loading all of ShortRead, we just specify the exact methods
within ShortRead that dada2 will use. It's much cleaner, and avoids some
collisions among the packages that dada2 depends on.


Reply to this email directly or view it on GitHub
#28 (comment).

joey711 added a commit that referenced this issue Aug 7, 2015
Down to just “missing doc” warnings, and a code-example error.

Helps to address the following issues

#20

#28
@joey711 joey711 mentioned this issue Aug 7, 2015
@joey711 joey711 closed this as completed Aug 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants