Few lingering check issue

Issue #8 resolved
Jason Vander Heiden created an issue

Hey, looks like you got most of the check issues resolved. Awesome! Couple simple things left, so I thought I make a new issue rather than reopen the old one. What's left looks pretty simple (way less effort than all the NSE stuff):

1. Easy

> codetools::checkUsagePackage('tigger')
plotNovel: local variable ‘min_frac’ assigned but may not be used (/home/jason/workspace/igpipeline/tigger/R/functions.R:437)

2. The following actually all have the same fix. Which is to use @importFrom, rather than @import, to import only the functions you need (or think you'll need) from dplyr and tidyr. You can probably just cut and paste what's in shazam/shazam.R and I'd wager it'd be sufficient, but if it's not, and you're not sure what functions you need, then you can just remove the imports and run codetools::checkUsagePackage('tigger') to see what it complains about missing. We'll also want to move dplyr and tidyr from Depends to Imports in DESCRIPTION.

Example fix:

#' @importFrom  dplyr       do n desc %>%
#'                          as_data_frame data_frame data_frame_
#'                          bind_cols bind_rows combine
#'                          filter filter_ select select_ arrange arrange_
#'                          group_by group_by_ ungroup
#'                          mutate mutate_ transmute transmute_
#'                          rename rename_ summarize summarize_
#' @importFrom  tidyr       gather gather_ spread spread_

Errors:

* checking whether package tigger can be installed ... WARNING
Found the following significant warnings:
  Warning: replacing previous import by tidyr::%>% when loading tigger
Warning message:
character(0) 
Loading required package: dplyr

Attaching package: dplyr

The following objects are masked from package:stats:

    filter, lag

The following objects are masked from package:base:

    intersect, setdiff, setequal, union

3. Broken examples.

* checking examples ... ERROR
Running examples in tigger-Ex.R failed
The error most likely occurred in:

> base::assign(".ptime", proc.time(), pos = "CheckExEnv")
> ### Name: genotypeFasta
> ### Title: Return the nucleotide sequences of a genotype
> ### Aliases: genotypeFasta
> 
> ### ** Examples
> 
> # Load example data
> data(germline_ighv)
> data(novel_df)
Warning in data(novel_df) : data set novel_df not found
> data(genotype)
Warning in data(genotype) : data set genotype not found
>                      
> # Find the sequences that correspond to the genotype
> genotype_seqs = genotypeFasta(genotype, germline_ighv, novel_df)
Error in nrow(novel_df) : object 'novel_df' not found
Calls: genotypeFasta -> nrow

4. I fixed a minor issue with the vignette header from R CMD check. I also added an .Rbuildignore to the repo to fix another warning, so if you have extra stuff in this file, you might want to save it before pulling.

5. I didn't check it on win-builder, but this would be the final check. It needs to pass on the current R-release, the current R-devel, as well as the minimum we set in DESCRIPTION. New exciting errors always pop up checking against R-devel. :)

Comments (6)

  1. Daniel Gadala-Maria

    Thanks, Jason!

    1. Strange...not sure why it didn't come up when I ran check. Easy enough to fix.

    2. I looked into this. I was scared I'd miss a function/I think I use some functions shazam and alakazam don't. I guess I'll have to comb my code for instances of all the functions I use, as well as be wary if I add any code with dplyr functions to make sure i import them. Seems like it would be easier to just load the package and warn the user. (If we load the function with overlapping and don't warn the user isn't that bad, since they might use the wrong function on accident?)

    3. This worked for me. I must have missed uploading the new data to the repo; will do that now.

    4. Ok, thanks

    5. Oh, boy...how fun...

  2. Jason Vander Heiden reporter

    2. Yeah, it would be easier, but the CRAN policies are pretty strict. If we move them to Imports and use @importFrom, then it shouldn't be a problem as the functions aren't loaded into the global namespace, but rather the package's local namespace. So, you should be able to load plyr after tigger without breaking tigger, despite tigger needing the dplyr version of do (or whatever). The warning is automatic upon conflict, so if we aren't explicitly suppressing the warnings, then we shouldn't get a conflict. The conflicts are also the cause of this completely unhelpful warning:

    Warning message:
    character(0) 
    

    (That took me forever to figure out.)

    5. The CRAN submission can be a huge hassle, but if get it to the point were we have only one NOTE about our license not being a FOS license, then we only have to add a little comment in the submission and it should go smoothly.

  3. Daniel Gadala-Maria

    1. Turns out min_frac was needed! Haha, added it back.

    2. Should be good now except I can't use 'glimpse' in my vignette any more. Is there a way around this?

    5. Will try to run this tonight...

  4. Jason Vander Heiden reporter

    Cool. I'll rebuild the docs for http://tigger.readthedocs.io. Can you skim over them and make sure everything is in order? Oh, and don't delete mkdocs.yml - we need that for the website to work.

    Also, I'd remove the v0.2.5 tag for now, and tag the version after it has been officially accepted by CRAN, because they might need us to make some changes after we submit.

  5. Jason Vander Heiden reporter

    Ah, I see what happened... Looks like you accidentally deleted both mkdocs.yml and the vignette from the repo during a merge. I'll fix it... Rebuilt docs should be up after my commit today.

  6. Log in to comment