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

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

Closed
17 tasks done
whedon opened this issue Nov 14, 2016 · 18 comments
Closed
17 tasks done
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review rOpenSci Submissions associated with rOpenSci

Comments

@whedon
Copy link

whedon commented Nov 14, 2016

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

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

  • 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

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

Functionality

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

Documentation

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

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

whedon commented Nov 14, 2016

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
Copy link
Member

arfon commented Nov 14, 2016

@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
Copy link
Member

arfon commented Nov 16, 2016

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

@sewardlee337
Copy link

sewardlee337 commented Nov 16, 2016

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

@arfon
Copy link
Member

arfon commented Nov 21, 2016

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

@jsta
Copy link
Member

jsta commented Nov 30, 2016

Reviewer questions

Conflict of interest

  • 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

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

The LICENSE file is incomplete.

  • Version: Does the release version given match the GitHub release (v1.0.1)?

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

  • Authorship: Has the submitting author (sewardlee337) made major contributions to the software?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • 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.

  • Performance: Have any performance claims of the software been confirmed?

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
  • 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.

  • 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

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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • 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
Copy link
Member

jsta commented Nov 30, 2016

@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
Copy link

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

@sewardlee337
Copy link

sewardlee337 commented Nov 30, 2016

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
Copy link
Member

jsta commented Dec 2, 2016

@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

@arfon
Copy link
Member

arfon commented Dec 3, 2016

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

@arfon arfon added the accepted label Dec 3, 2016
@sewardlee337
Copy link

Thank you @jsta and @arfon!

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

@arfon
Copy link
Member

arfon commented Dec 4, 2016

@whedon commands

@whedon
Copy link
Author

whedon commented Dec 4, 2016

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

🚧 Important 🚧

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
Copy link
Member

arfon commented Dec 4, 2016

@whedon set 10.5281/zenodo.192466 as archive

@whedon
Copy link
Author

whedon commented Dec 4, 2016

OK. 10.5281/zenodo.192466 is the archive.

@arfon
Copy link
Member

arfon commented Dec 5, 2016

@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 ⚡ 🚀 💥

@arfon arfon closed this as completed Dec 5, 2016
@sewardlee337
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review rOpenSci Submissions associated with rOpenSci
Projects
None yet
Development

No branches or pull requests

4 participants