[REVIEW]: finreportr - Financial Data from U.S. Securities and Exchange Commission

whedon commented 7 years ago

Submitting author: @sewardlee337 (Seward Lee) Repository: https://github.com/sewardlee337/finreportr Version: v1.0.1 Editor: @arfon Reviewer: @jsta Archive: 10.5281/zenodo.192466

Status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/128c974cac2dcf92b673c66f39a2c93e"><img src="http://joss.theoj.org/papers/128c974cac2dcf92b673c66f39a2c93e/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/128c974cac2dcf92b673c66f39a2c93e/status.svg)](http://joss.theoj.org/papers/128c974cac2dcf92b673c66f39a2c93e)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer questions

Conflict of interest

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (v1.0.1)?
[x] Authorship: Has the submitting author (sewardlee337) made major contributions to the software?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: Have any performance claims of the software been confirmed?

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

whedon commented 7 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks for JOSS.

For a list of things I can do to help you, just type:

@whedon commands

arfon commented 7 years ago

@jsta - many thanks for agreeing to review this submission. Please take a look at the reviewer guidelines and check off items on the checklist above as you progress.

arfon commented 7 years ago

@sewardlee337 - could you please move all of the references into the paper.bib file so that we can compile the paper?

sewardlee337 commented 7 years ago

@arfon - references have been added to paper.bib. Thanks!

arfon commented 7 years ago

@jsta - here's the compiled paper: 10.21105.joss.00119.pdf

jsta commented 7 years ago

Reviewer questions

Conflict of interest

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?

The LICENSE file is incomplete.

[x] Version: Does the release version given match the GitHub release (v1.0.1)?

As of my last check there has been no Github release.

[x] Authorship: Has the submitting author (sewardlee337) made major contributions to the software?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?

My run of the example README.md code for AnnualReports does not match the expected output. For example, when I run AnnualReports("GOOG") I get:

filing.name filing.date accession.no 1 10-K/A 2016-03-29 0001193125-16-520367 2 10-K 2016-02-11 0001652044-16-000012

rather than:

filing.name filing.date accession.no 1 10-K 2015-02-09 0001288776-15-000008 2 10-K 2014-02-12 0001288776-14-000020 3 10-K 2013-01-29 0001193125-13-028362 4 10-K/A 2012-04-23 0001193125-12-174477 5 10-K 2012-01-26 0001193125-12-025336 6 10-K 2011-02-11 0001193125-11-032930 7 10-K 2010-02-12 0001193125-10-030774 8 10-K 2009-02-13 0001193125-09-029448 9 10-K 2008-02-15 0001193125-08-032690 10 10-K 2007-03-01 0001193125-07-044494 11 10-K 2006-03-16 0001193125-06-056598 12 10-K 2005-03-30 0001193125-05-065298

If I try the roxygen example code for GetCashFlow("FB", 2016) I get the following error:

Error in fileFromCache(file) : Error in download.file(file, cached.file, method = "auto", quiet = !verbose) : cannot download all files

In addition: Warning message: In download.file(file, cached.file, method = "auto", quiet = !verbose) : URL 'https://www.sec.gov/Archives/edgar/data/1326801/000132680116000043/fb-20151231.xsd': status was '404 Not Found'

I was able to fix this error by modifying the dependency XBRL to call download.file(method = "curl"). See https://github.com/jsta/XBRL/commit/9c42ada9947aadfbc2f2fe64705cabab9eddca40.

[x] Performance: Have any performance claims of the software been confirmed?

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?

The automated testthat tests are extremely minimal. They only test one function. However, the manual steps described in the roxygen documentation and README.md should be sufficient to verify package functionality.

[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

At the very least you should fill out a BugReports section in the DESCRIPTION. Some package authors also choose to include a CONTRIBUTING.md. See https://github.com/ropensci/rnoaa/blob/master/CONTRIBUTING.md for an example.

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?

It seems to me that not everyone necessarily has an affiliation but going by the letter-of-the-law there is no affiliation listed in paper.md.

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

Miscellaneous

I'm not sure how you are building this package for CRAN and avoiding the warnings about non-standard top-level files like Read-and-delete-me and .RData. These should probably be removed or appended to Rbuildignore.

When I run goodpractice::gp() on the package using goodpractice I get the following notes:

✖ omit "Date" in DESCRIPTION. It is not required and it gets invalid quite often. A build date will be added to the package when you perform R CMD build on it. ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug tracker. Many online code hosting services provide bug trackers for free, https://github.com, https://gitlab.com, etc. ✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80 characters wide. Try make your lines shorter than 80 characters

R/annual_reports.R:18:1
R/annual_reports.R:20:1
R/annual_reports.R:53:1
R/backend_utilities.R:19:1
R/backend_utilities.R:42:1
... and 19 more lines

✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and 1:NCOL(...) expressions. They are error prone and result 1:0 if the expression on the right hand side is zero. Use seq_len() or seq_along() instead.

R/get_financial.R:68:17

✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import only the specific functions you need.

jsta commented 7 years ago

@arfon I ran into somewhat of a show-stopper bug with one of finreportr's dependencies (see my comment above about XBRL). It seems like a bug in a dependency is out of the present author's control. I was able to patch it myself to test the finreportr functions but the everyday user may run into frustration. How should this be handled for the review?

sewardlee337 commented 7 years ago

@jsta - Thank you for the advice! I will start implementing some of these fixes.

sewardlee337 commented 7 years ago

Latest updates to finreportr per @jsta's suggestions and JOSS review checklist:

BugReports added to DESCRIPTION and Feedback section added to README.md
MIT License full text added to LICENSE per JOSS requirement
finreportr ver. 1.0.1 released on GitHub
Long lines of code cut down where contextually appropriate
Deleted top-level files Read-and-delete-me and .RData

jsta commented 7 years ago

@arfon I think this submission is good to go as far as acceptance pending your opinion on the XBRL (dependency of the present submission) bug described at: https://github.com/sewardlee337/finreportr/issues/1

arfon commented 7 years ago

@arfon I think this submission is good to go as far as acceptance pending your opinion on the XBRL (dependency of the present submission) bug described at: sewardlee337/finreportr#1

Thanks @jsta. I agree this is good to move forward.

@sewardlee337 - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

sewardlee337 commented 7 years ago

Thank you @jsta and @arfon!

The DOI of the software archive is http://doi.org/10.5281/zenodo.192466

arfon commented 7 years ago

@whedon commands

whedon commented 7 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the reviewer of this submission
@whedon assign @username as reviewer

# List the GitHub usernames of the JOSS editors
@whedon list editors

# List of JOSS reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

:construction: Important :construction:

This is all quite new. Please make sure you check the top of the issue after running a @whedon command (you might also need to refresh the page to see the issue update).

arfon commented 7 years ago

@whedon set 10.5281/zenodo.192466 as archive

whedon commented 7 years ago

OK. 10.5281/zenodo.192466 is the archive.

arfon commented 7 years ago

@jsta many thanks for the review here.

@sewardlee337 - your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00119 :zap: 🚀 💥

sewardlee337 commented 7 years ago

Thank you @arfon and @jsta for reviewing and accepting my paper!!

openjournals / joss-reviews