smped commented 5 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Repository: https://github.com/UofABioinformaticsHub/ngsReports

Confirm the following by editing each check box to '[x]'

[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.

I am familiar with the essential aspects of Bioconductor software management, including:

[x] The 'devel' branch for new packages and features.
[x] The stable 'release' branch, made available every six months, for bug fixes.
[x] Bioconductor version control using Git (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 5 years ago

Hi @steveped

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: ngsReports
Version: 0.99.0
Date: 2018/02/01
Title: Load FastqQC reports and other NGS related files
Authors@R: c(
    person("Steve", "Pederson", , "stephen.pederson@adelaide.edu.au", c("aut", "cre")),
    person("Christopher", "Ward", , "christopher.ward@adelaide.edu.au", c("aut")),
    person("Thu-Hien", "To", , "tothuhien@gmail.com", c("aut"))
    )
Maintainer: Steve Pederson <stephen.pederson@adelaide.edu.au>
Description: Load raw data from FastQC reports and other NGS output summaries into R.
URL: https://github.com/UofABioinformaticsHub/ngsReports
BugReports: https://github.com/UofABioinformaticsHub/ngsReports
License: LGPL (>= 3)
Encoding: UTF-8
Depends:
  R (>= 3.5.0),
  BiocGenerics,
  fastqcTheoreticalGC,
  ggplot2,
  tibble (>= 1.3.1)
Imports:
  Biostrings,
  checkmate,
  dplyr (>= 0.7.5),
  ggdendro,
  grDevices, 
  grid,
  lubridate,
  methods,
  parallel,
  plotly,
  readr,
  reshape2,
  rmarkdown,
  Rsamtools,
  scales,
  ShortRead,
  stats,
  stringr,
  tidyr,
  tidyselect (>= 0.2.3),
  utils,
  viridisLite,
  zoo
Remotes: mikelove/fastqcTheoreticalGC
LazyData: true
RoxygenNote: 6.1.1
Collate: 
    'validationFunctions.R'
    'FastqcDataList.R'
    'FastqcFileList.R'
    'FastqcFile.R'
    'AllGenerics.R'
    'FastqcData.R'
    'AdapterContent.R'
    'Basic_Statistics.R'
    'Kmer_Content.R'
    'Overrepresented_sequences.R'
    'Per_base_N_content.R'
    'Per_base_sequence_content.R'
    'Per_base_sequence_quality.R'
    'Per_sequence_GC_content.R'
    'Per_sequence_quality_scores.R'
    'Per_tile_sequence_quality.R'
    'PwfCols.R'
    'Sequence_Duplication_Levels.R'
    'Sequence_Length_Distribution.R'
    'TheoreticalGC.R'
    'Total_Deduplicated_Percentage.R'
    'Version.R'
    'aaa.R'
    'addPercent.R'
    'data.R'
    'emptyPlot.R'
    'exportOverrepresented.R'
    'extract.R'
    'fileName.R'
    'gcFromFasta.R'
    'getColours.R'
    'getFastqcData.R'
    'getSummary.R'
    'importBowtieLogs.R'
    'importDuplicationMetrics.R'
    'importHisat2Logs.R'
    'importStarLogs.R'
    'isCompressed.R'
    'makeDendrogram.R'
    'makeLabels.R'
    'makeSidebar.R'
    'maxAdapterContent.R'
    'path.R'
    'plotAdapterContent.R'
    'plotBaseQualities.R'
    'plotDuplicationLevels.R'
    'plotGcContent.R'
    'plotKmers.R'
    'plotNContent.R'
    'plotOverrepresentedSummary.R'
    'plotReadTotals.R'
    'plotSequenceContent.R'
    'plotSequenceLengthDistribution.R'
    'plotSequenceQualities.R'
    'plotSummary.R'
    'pwf.R'
    'readTotals.R'
    'renderDendro.R'
    'runFastQC.R'
    'scale_fill_pwf.R'
    'splitByTab.R'
    'writeHtmlReport.R'
VignetteBuilder: knitr
Suggests: 
  BiocStyle, 
  knitr, 
  pander,
  testthat
biocViews: QualityControl, ReportWriting

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

bioc-issue-bot commented 5 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

2f72ccc Revert after temp changes 26173d8 Rebuilt getting GC content from fasta file so it n... 84bbcb1 Bumped Version for resubmission

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

f9105f2 Removed spurious import

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

33575e9 Dammit. Fixed NAMESPACE this time

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

52667e8 OK. Got it this time. It was in the Depends field

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

d4d117b - Changed suffix of example files from .log to ....

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR, WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

a50ff6a Removed: - TTAT example files - usePlotly = TRUE e... 10326e1 Changed travis for r-devel & removed osx build 0d55302 Updated version number for merging branches

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR, WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

smped commented 5 years ago

Hi @LiNk-NY , The latest build has two flags.

The ERROR is on celaya2 & it looks to me like this is a Cairo (i.e. known) error. Can this be ignored or do I need to find a way rectify this?
The second flag is a WARNING on tokay2, where the R CMD check time is 5 minutes 14.68 seconds. I assume this needs 14.69 seconds chopped off it? Cheers, Steve

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

e7fecbc Removed example & dropped >1 second from check 14c8052 - Removed redundant 'else' conditionals to try spe...

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "TIMEOUT, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

smped commented 5 years ago

Hi again @LiNk-NY. I'm pretty convinced that the WARNING on celaya2 is Cairo-related (which apparently has no OSX driver for r-devel). I can easily get rid of them by dropping the tests that rely on rendering a plotly object, but that's really inappropriate. However, I cannot reproduce that TIMEOUT using the bioc-devel2 docker or my local Ubuntu machine. I suspect there was a glitch on malbec2 during that build. Is there any way to re-initiate the build without me pushing a trivial commit? Thanks, Steve

lshep commented 5 years ago

celaya2 - CRAN will eventually provide the binaries for this R version - you can ignore for now and we will review based on the other two platforms I am kicking a manual build off to try to remedy the timeout - we have been having some intermittent connectivity issues to one of our servers causing this issue - we are working on it but for now can also be ignore - @LiNk-NY

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

smped commented 5 years ago

Thanks @lshep . Much appreciated & it looks like that glitch was the problem.

LiNk-NY commented 5 years ago

Hi Steve, @steveped

Apologies for the delay. Thanks for bearing with our SPB / CRAN troubles. I will get to your package within the next couple of days.

Best, Marcel

smped commented 5 years ago

Thanks Marcel @LiNk-NY . I fear this is a surprisingly big package to look through too, so my apologies for adding another pile to your workload

LiNk-NY commented 5 years ago

Hi Steve, @steveped Thank you for your submission. Please see the review below.

Reply in the issue if you have any questions.

Best, Marcel

ngsReports #985

DESCRIPTION

Point your BugReports to the issues page
Provide a more complete Description field
Avoid using any GitHub repositories (remove the Remotes field)

NAMESPACE

Stick to either snake_case or camelCase (snake case is recommended for tidyverse interfacing packages, otherwise use camelCase)
Consider shortening function names, long function names can be cumbersome and awkward for the user unless they are established file standards
Consider using plot methods on classes so that you don't have to append each plot method with plot*

vignettes

Remove the R file from the folder
Respect the 80 column width limits. It improves readability and maintainership
Use system.file to provide the template.Rmd
Do you need to make a distinction between a single file and a list of files? Can they be accomodated by a single FastqcFileList class?
It's easy to get lost in the 'Generating Plots' section. Perhaps it would be helpful to categorize groups of functions.

R

As mentioned earlier, you can reduce FastqcFile and FastqcFileList into a single class. This would avoid you the repitition of methods for both classes, otherwise use inheritance.
Group files where possible to reduce the number of files to maintain, such as helper functions
I don't quite understand why you need to create a generic from the class name instead of a simple constructor function (FastqcFile).
Why not use the established path generic rather than creating fileName?
Create a coercion method instead of a class method to move from one class representation to another
Only set methods for classes that are your own and use the ANY class for setting methods for vectors.
It looks like you're not taking advantage of existing Bioconductor classes. Your main class in use is the tibble.
Is there functionality that you could use from the ShortRead package?

smped commented 5 years ago

Hi Marcel,

Thanks for all of those comments and there's lots for us to work on there and we'll get onto it. There were a few things we were unclear on though. If possible, could you try to help us understand these comments?

I don't quite understand why you need to create a generic from the class name instead of a simple constructor function (FastqcFile).

I'm a S4 noob obviously. This was written to enable construction of a FastqcFile object. Are you meaning that we should do this using a stand-alone function rather than a generic? Is there an example in another package you can easily recall that we can base this off? I've tried to copy from existing packages like Rsamtools and ShortRead as much as possible already, but there was a lot in those that went over my head.

Create a coercion method instead of a class method to move from one class representation to another

Do you mean from FastqcFile to FastqcData? We have coercion methods from things like general lists to FastqcFileList objects, but are a bit unclear as to which specific classes you mean this to apply to. Currently the coercion from FastqcFile* to FastqcData* is done via the function getFastqcData() so is this inappropriate?

Only set methods for classes that are your own and use the ANY class for setting methods for vectors.

Does this mean that we shouldn't define methods for existing Bioconductor classes? This sounds like it defeats the entire point of Bioconductor, so we're probably misunderstanding you here.
Are you referring to all the times we've set a method for the signature "character"? I based our code on this https://github.com/Bioconductor/ShortRead/blob/master/R/methods-FastqFile.R, but have missed the ANY class. Mainly because I'm an S4 noob (again) and can't figure out why this is needed in our context.

One additional question I had which may clear up a few other questions you had too, is that much of the design was based around the shiny app which we removed because we couldn't figure out how to include it in a Bioc-type manner. We've just found https://github.com/jdgagnon/plotGrouper which gave us a good solution to this. Would including this app in inst/application be an appropriate thing to do at this point, or would you actively discourage this?

Thanks again and we'll get going on all the changes that we do understand in the meantime.

Cheers,

Steve

LiNk-NY commented 5 years ago

I don't quite understand why you need to create a generic from the class name instead of a simple constructor function (FastqcFile).

I'm a S4 noob obviously. This was written to enable construction of a FastqcFile object. Are you meaning that we should do this using a stand-alone function rather than a generic? Is there an example in another package you can easily recall that we can base this off? I've tried to copy from existing packages like Rsamtools and ShortRead as much as possible already, but there was a lot in those that went over my head.

Yes, it should be a standalone constructor function that performs the necessary input checks and calls new towards the end.

Create a coercion method instead of a class method to move from one class representation to another

Do you mean from FastqcFile to FastqcData? We have coercion methods from things like general lists to FastqcFileList objects, but are a bit unclear as to which specific classes you mean this to apply to. Currently the coercion from FastqcFile* to FastqcData* is done via the function getFastqcData() so is this inappropriate?

Yes, with S4 classes, you can create a setAs method to coerce from one class to another instead of using getFastqcData. This is a more standard way of coercing classes that are your own.

Only set methods for classes that are your own and use the ANY class for setting methods for vectors.

Does this mean that we shouldn't define methods for existing Bioconductor classes? This sounds like it defeats the entire point of Bioconductor, so we're probably misunderstanding you here.

That is correct, you don't want to step on other classes' toes. They should have a set of defined methods within their package and you should only create and modify methods within yours. The point of Bioconductor is to re-use other classes where appropriate such as SummarizedExperiment, FastqFile, etc.

Are you referring to all the times we've set a method for the signature "character"? I based our code on this https://github.com/Bioconductor/ShortRead/blob/master/R/methods-FastqFile.R, but have missed the ANY class. Mainly because I'm an S4 noob (again) and can't figure out why this is needed in our context.

Consider input other than character and include what to do with these class types by using an ANY class method. You can filter out the character inputs in the ANY method or you can separately create a character method for the function.

One additional question I had which may clear up a few other questions you had too, is that much of the design was based around the shiny app which we removed because we couldn't figure out how to include it in a Bioc-type manner. We've just found https://github.com/jdgagnon/plotGrouper which gave us a good solution to this. Would including this app in inst/application be an appropriate thing to do at this point, or would you actively discourage this?

I would encourage you to keep the functionality in a separate package. It is best to have a cohesive package that works on a single / central representation. You can also consider submitting the shiny package to Bioconductor as well.

Best, Marcel

LiNk-NY commented 5 years ago

Hi Steve, @steveped Please provide an update on the status of your package. Otherwise, I'd be forced to close the issue.

Best regards, Marcel

smped commented 5 years ago

Hi Marcel @LiNk-NY ,

Nearly there. Your comments were asking considerable rewrite of the package and we've still got 3 points on your list left to address. Hoping to have them done by the end of the week. Latest commit is at: https://github.com/UofABioinformaticsHub/ngsReports/tree/dev.

When does the next release of BioC close? I'm desperately trying to get it done by then.

All the best,

Steve

LiNk-NY commented 5 years ago

Hi Steve, @steveped

Here is the release schedule http://bioconductor.org/developers/release-schedule/. Please have the changes done by mid-April to be safe.

Best, Marcel

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

48df2fb Added Marcel's comments in BiocTODO.md d207f4e First few easy issues done 6ddc8d8 Minor changes to DESCRIPTION and vignette bd72103 Renamed exportOverrespresented() as overRep2Fas... [31d6e88](https://github.com/UofABioinformaticsHub/ngsReports/commit/31d6e88e52a8aa9a057f3630cd28a7dd79892e0c) RenamedgetGcDistribution()asgetGcDistn() [f6d3fc4](https://github.com/UofABioinformaticsHub/ngsReports/commit/f6d3fc464530b6a93517836fa2dfa086482fd584) RenamedplotOverrepresentedSummary()asplotOve... de94e20 - Renamed plotSequenceQualities() to plotSeqQua... [ab8b429](https://github.com/UofABioinformaticsHub/ngsReports/commit/ab8b429d513bcbac55877f97c3bcbeae4104ef30) - RenamedplotDuplicationLevels()toplotDupLev... 11575d2 - Renamed importDuplicationMetrics() to importD... [d91c2b8](https://github.com/UofABioinformaticsHub/ngsReports/commit/d91c2b8cad7e59172a96c0e33ad1a63acc4cbf58) Corrected function names on report template [0420397](https://github.com/UofABioinformaticsHub/ngsReports/commit/0420397b104420d67da8bdb90262977f8ae5013f) RenamedmakeDendrogram()asmakeDendro() [e0cf2b3](https://github.com/UofABioinformaticsHub/ngsReports/commit/e0cf2b31e3a8e4d6b5bbafe403568c28ddd5a8d8) Minor changes to line formatting in vignette [b3ead3d](https://github.com/UofABioinformaticsHub/ngsReports/commit/b3ead3dce8065d7424b5a0f3063f42c58f52b886) DefinedgetModule()method for extracting all mo... [5fc9fbd](https://github.com/UofABioinformaticsHub/ngsReports/commit/5fc9fbdd1df31390fed4cad581cc5d245cd901c4) MigratedBasic_Statistics()togetModule(module... 95dd5a0 Migrated Per_base_sequence_quality() to getModu... [b38bfbf](https://github.com/UofABioinformaticsHub/ngsReports/commit/b38bfbfcfef47b1ba4810c145d7bd88f3f098002) Migrated more module functions [2e004cf](https://github.com/UofABioinformaticsHub/ngsReports/commit/2e004cfab1cd81a7aafc74cb70729cba37108092) MigratedPer_base_sequence_content()togetModu... 69e8718 Migrated Per_sequence_GC_content() to getModule... [df02df7](https://github.com/UofABioinformaticsHub/ngsReports/commit/df02df7fc5075d1c2e99143c54f7630ad1ad7841) MigratedPer_base_N_content()togetModule() [1f0845c](https://github.com/UofABioinformaticsHub/ngsReports/commit/1f0845c7bf9994471d41a2cca9f7f1a2bc60a9ea) MigratedOverrepresentedsequences(),Sequence... 4364067 Migrated Adapter_Content() to getModule() e6e043f Migrated Kmer_Content() to getModule() 79a1f5f Build after forgetting... 319f3f0 All modules now migrated to getModule() e06b05f Migrated importBowtieLogs() to importNgsLogs() c450483 Migrated importHisat2Logs() and importBowtie2Lo... [1609aa1](https://github.com/UofABioinformaticsHub/ngsReports/commit/1609aa1470b4de2a550312fc278f2fc56fe3aaa5) MigratedimportStarLogs()toimportNgsLogs(). ... [93e8e08](https://github.com/UofABioinformaticsHub/ngsReports/commit/93e8e083ae6379ed2fef92d538d348865924288a) RemovedfileName()method from FastqcFile* objec... [84972fa](https://github.com/UofABioinformaticsHub/ngsReports/commit/84972fa7fb867e785619950a82f3d3ec918c27e2) MigratedimportDuplicationMetrics()toimportNg... a5df8bd Moved helper functions to a single file 0bd93b8 Revised Vignette e9ae54c Tweaks to vignette 0c6428e Fixed BiocCheck NOTES 6c5a5d7 Tidied up numerous ggplot2 calls a27a60d Minor grammar changes 1a3fc12 Removed Generics for FastqcFile and FastqcFileList e81ed36 Wrote beginnings of more correct S4 coercion metho... c4bf508 Tested basic coercion and made minor changes to im... 9ec3491 - Moved FastqcFile to a private class with private... 21e306b Fixed examples after R CMD check failure 00cde3d Removed all calls to getFastqcData() and plot me... 0c22529 Removed getFastqcData() & added tests for a vali... 3c41300 Changed version() method to fqcVersion() for s... 44bb204 Added names to FastqcDataList objects 661bbc3 Changed fileName() to fqName() b91ee57 Updated NEWS and corrected imports bda5f2b - Defined methods for ANY - Updated a few man page... 00ce878 Reimplemented getGcDistn() as an S3 method & cor... c50385e Migrated genomes() and transcriptomes() to the... 2b6a159 - Increased test coverage - Checked all ANY method... 56432a4 Final version for Travis testing before merging br... e1b8a16 Version bump & NEWS update

smped commented 5 years ago

Hi Marcel @LiNk-NY .

There's a push being tested & built now. I thought I'd post your comments & my thoughts as well. Hope they make sense. See below.

Cheers,

Steve

DESCRIPTION

[x] Point your BugReports to the issues page
[x] Provide a more complete Description field
[x] Avoid using any GitHub repositories (remove the Remotes field)

NAMESPACE

[x] Stick to either snake_case or camelCase (snake case is recommended for tidyverse interfacing packages, otherwise use camelCase)
- Understood. This prompted a significant rewrite so that individual modules are now accessed using getModule() instead of having their own function. Function names are now all camelCase. Import methods have now all been unified too.
- Similarly I have dispensed with the numerous import*() functions into a single importNgsLogs() function.
[x] Consider shortening function names, long function names can be cumbersome and awkward for the user unless they are established file standards
- I think we've found a good trade-off now between clear function names and brevity
[x] Consider using plot methods on classes so that you don't have to append each plot method with plot*
- I think there's a small misunderstanding of the object classes here. The main object classes of concern are FastqcData and FastqcDataList objects, which themselves contain the standard modules generated by FastQC as individual elements (or modules), after parsing into R. The plot* methods apply to different modules within each object and as such these are not different object classes. Developing individual plot methods for each module as you suggest would require the declaration of each module as a new object class, which doesn't seem appropriate to me and would involve a very significant rewrite of the package. The different plot* functions for these modules within a FastqcData* object also require numerous different graphical arguments and we believe combining these into a single plot() function would significantly reduce usability.

Vignettes

[x] Remove the R file from the folder
[x] Respect the 80 column width limits. It improves readability and maintainership
[x] Use system.file to provide the template.Rmd
- I don't really understand this comment. This is how the file is referred to internally. Beyond that it's not referred to here with anything beyond a toy example path. What am I missing?
[x] Do you need to make a distinction between a single file and a list of files? Can they be accomodated by a single FastqcFileList class?
- I can see your point here & I've tried to alleviate the confusion with these classes. I've made the FastqcFile class into a private class (.FastqcFile), essentially acting just as a checking step allowing for quick erroring. I've also removed the FastqcFileList class so the only classes shown to the user are FastqcData and FastqcDataList.
[x] It's easy to get lost in the 'Generating Plots' section. Perhaps it would be helpful to categorize groups of functions.
- This has been significantly rewritten removing less common plots and highlighting the differences between applying plot* methods to a FastqcData and FastqcDataList object.

R

[x] As mentioned earlier, you can reduce FastqcFile and FastqcFileList into a single class. This would avoid you the repitition of methods for both classes, otherwise use inheritance.
- I have removed the FastqcFileList and set FastqcFile as a private helper class
[x] Group files where possible to reduce the number of files to maintain, such as helper functions
[x] I don't quite understand why you need to create a generic from the class name instead of a simple constructor function (FastqcFile).
- Thanks for pointing this out. I'd misunderstood the normal approach for this. Of course the constructors are now FastqcData() and FastqcDataList()
[x] Why not use the established path generic rather than creating fileName?
- We actually have used the path generic but I see the point of confusion. The function fileName returned the name of the underlying Fastq file the report was generated from, and has now been changed to fqName() to avoid any confusion
[x] Create a coercion method instead of a class method to move from one class representation to another
[ ] Only set methods for classes that are your own
- I haven't done this based on Martin's response to my question on the Bioc-devel list.
[x] Use the ANY class for setting methods for vectors.
- Hopefully I've done this correctly now. I've basically just given them all an error message for classes not implemented
[ ] It looks like you're not taking advantage of existing Bioconductor classes. Your main class in use is the tibble.
- DataFrame objects play poorly with ggplot2 and as that is the primary role of this package, I believe this would impede ease of use. Every parsed DataFrame would require an extra line of code converting back to a data.frame before it was able to be used for plotting by users, and would serve no purpose beyond being user-unfriendly and adding numerous lines of unnecessary code to existing plot functions. I could possibly change these outputs to data.frame objects, but they're not as user-friendly & honestly I can't see any functional advantage this would give to package users.
[x] Is there functionality that you could use from the ShortRead package?
- Not really. This current package is about simply parsing outputs from stand-alone tools and not about performing any new analysis (with the exception of runFastQC()). It's possible that at a later date, plot methods may be plausibly implemented for ShortRead outputs but that is not our current intent. However, as mentioned, we have used the FastqFile and FastqFileList object classes from ShortRead (and BamFile/BamFileList classes from Rsamtools), and the structure of these formed the basis of package design.

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

24c3157 Removed Maintainer field from DESCRIPTION

bioc-issue-bot commented 5 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

LiNk-NY commented 5 years ago

Hi Steve, @steveped Thank you for your submission and contribution to Bioconductor. Your package has been accepted. You can ignore the 5 min warning on the windows builder.

Best regards, Marcel

bioc-issue-bot commented 5 years ago

Your package has been accepted. It will be added to the Bioconductor Git repository and nightly builds. Additional information will be posed to this issue in the next several days.

Thank you for contributing to Bioconductor!

smped commented 5 years ago

Thanks Marcel . Great news!

Steve

mtmorgan commented 5 years ago

The master branch of your GitHub repository has been added to Bioconductor's git repository.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/steveped.keys is not empty), then no further steps are required. Otherwise, do the following:

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("ngsReports"). The package 'landing page' will be created at

https://bioconductor.org/packages/ngsReports

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.

Bioconductor / Contributions

ngsReports Submission #985

ngsReports #985

DESCRIPTION

NAMESPACE

vignettes

R

DESCRIPTION

NAMESPACE

Vignettes

R