ngsReports Submission

[smped]

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]'

• I understand that by submitting my package to Bioconductor,
  the package source and all review commentary are visible to the
  general public.

• I have read the Bioconductor Package Submission
  instructions. My package is consistent with the Bioconductor
  Package Guidelines.

• 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.

• My package addresses statistical or bioinformatic issues related
  to the analysis and comprehension of high throughput genomic data.

• 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:

• The 'devel' branch for new packages and features.
• The stable 'release' branch, made available every six
  months, for bug fixes.
• 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]

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]

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]

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]

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]

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]

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

f9105f2 Removed spurious import

[bioc-issue-bot]

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]

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

33575e9 Dammit. Fixed NAMESPACE this time

[bioc-issue-bot]

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]

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

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

[bioc-issue-bot]

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]

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

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

[bioc-issue-bot]

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]

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]

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]

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]

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

1. 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?
2. 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]

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]

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]

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]

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]

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]

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

[LiNk-NY]

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]

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]

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]

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]

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]

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]

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]

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]

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) Renamed getGcDistribution()asgetGcDistn()[f6d3fc4](https://github.com/UofABioinformaticsHub/ngsReports/commit/f6d3fc464530b6a93517836fa2dfa086482fd584) RenamedplotOverrepresentedSummary()asplotOve...
de94e20 - Renamed plotSequenceQualities() to plotSeqQua... [ab8b429](https://github.com/UofABioinformaticsHub/ngsReports/commit/ab8b429d513bcbac55877f97c3bcbeae4104ef30) - Renamed plotDuplicationLevels()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) Renamed makeDendrogram()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) Migrated Per_base_sequence_content()togetModu...
69e8718 Migrated Per_sequence_GC_content() to getModule... [df02df7](https://github.com/UofABioinformaticsHub/ngsReports/commit/df02df7fc5075d1c2e99143c54f7630ad1ad7841) Migrated Per_base_N_content()togetModule()[1f0845c](https://github.com/UofABioinformaticsHub/ngsReports/commit/1f0845c7bf9994471d41a2cca9f7f1a2bc60a9ea) MigratedOverrepresented_sequences(), 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) Migrated importStarLogs()toimportNgsLogs(). ... [93e8e08](https://github.com/UofABioinformaticsHub/ngsReports/commit/93e8e083ae6379ed2fef92d538d348865924288a) Removed fileName()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]

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

• 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)
  • 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.
• 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
• 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

• 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
  • 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?
• 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.
• 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

• 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
• 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).
  • Thanks for pointing this out. I'd misunderstood the normal approach for this. Of course the constructors are now FastqcData() and FastqcDataList()
• 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
• 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.
• 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.
• 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]

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]

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

24c3157 Removed Maintainer field from DESCRIPTION

[bioc-issue-bot]

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]

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]

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]

Thanks Marcel . Great news!

Steve

[mtmorgan]

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:

1. Add an SSH key to your github account
2. Submit your SSH key to Bioconductor

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.