openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
718 stars 38 forks source link

[REVIEW]: dtrackr: An R package for tracking the provenance of data #4707

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@robchallen<!--end-author-handle-- (Robert Challen) Repository: https://github.com/terminological/dtrackr Branch with paper.md (empty if default branch): main Version: 0.2.5-joss Editor: !--editor-->@ajstewartlang<!--end-editor-- Reviewers: @debruine, @craig-willis Archive: 10.5281/zenodo.7433514

Status

status

Status badge code:

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

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) by leaving comments 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 instructions & questions

@debruine & @craig-willis, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @ajstewartlang know.

Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest

Checklists

📝 Checklist for @craig-willis

📝 Checklist for @debruine

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.15 s (974.5 files/s, 210098.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                           104           5131           1342          18366
R                               13            348           1014           1416
SVG                              2              0              1            956
JavaScript                       6            109             62            442
CSS                              4             99             49            431
Rmd                              5            209            259            333
TeX                              2             34              0            240
Markdown                         5             79              0            237
YAML                             4             17             10             78
-------------------------------------------------------------------------------
SUM:                           145           6026           2737          22499
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 841

editorialbot commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1136/bmj.c332 is OK
- 10.1186/s12916-014-0241-z is OK
- 10.1371/journal.pmed.1001885 is OK
- 10.1371/journal.pmed.1001711 is OK
- 10.5121/IJDMS.2011.3207 is OK
- 10.1371/journal.pmed.0040297 is OK
- 10.7326/M18-1376 is OK
- 10.21105/joss.01686 is OK
- 10.1136/bmj.n579 is OK
- 10.2139/ssrn.4087373 is OK
- 10.1002/1097-024X(200009)30:11<1203::AID-SPE338>3.0.CO;2-N is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

craig-willis commented 2 years ago

Review checklist for @craig-willis

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

robchallen commented 2 years ago

I just wanted to mention that in the branch here under review, the README.md instructions say that "dtrackr" is not yet on CRAN.

This is not correct and the version in the joss review branch (release-0.2.4) of dtrackr is now on CRAN and can be installed with a install.packages("dtrackr") command.

I'm holding back on updating the documentation until I get comments back from you guys. Then I'll make any outstanding fixes update the documentation and push a 0.2.5 release through the CRAN submission process (with the updated installation instructions showing it is on CRAN).

I've just made the exact same mistake on another package I'm submitting to CRAN :-)

ajstewartlang commented 2 years ago

:wave: @debruine how's your review going?

robchallen commented 2 years ago

I've been working through @craig-willis comments, and made quite a few updates. These are in a new branch joss-fixes-0.2.4.9000. Would it be better to do additional review there?

debruine commented 2 years ago

I’m sorry I got swamped with work and am running late. I’ll start my review in this branch and aim to complete by Monday.

On 5 Oct 2022, at 00:47, Rob Challen @.***> wrote:

 I've been working through @craig-willis comments, and made quite a few updates. These are in a new branch joss-fixes-0.2.4.9000. Would it be better to do additional review there?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

ajstewartlang commented 2 years ago

I've been working through @craig-willis comments, and made quite a few updates. These are in a new branch joss-fixes-0.2.4.9000. Would it be better to do additional review there?

Sure - I'll set that as the review branch now.

ajstewartlang commented 2 years ago

@editorialbot set joss-fixes-0.2.4.9000 as branch

editorialbot commented 2 years ago

Done! branch is now joss-fixes-0.2.4.9000

craig-willis commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

debruine commented 2 years ago

Review checklist for @debruine

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

debruine commented 2 years ago

I'm finding this review process really interesting but a little overwhelming (I'm never sure I've read all the relevant instructions or are doing everything correctly). I'm nearly done checking the documentation for each function and hope to finish by Tuesday.

I just wanted to say that I love this package and have immediately started using it in my own data wrangling pipeline. It's very intuitive for a tidyverse user. I wish this were around 5 years ago when I was printing little tables after each wrangling step in a super-complex data join and filter.

robchallen commented 2 years ago

I just wanted to say that I love this package and have immediately started using it in my own data wrangling pipeline. It's very intuitive for a tidyverse user. I wish this were around 5 years ago when I was printing little tables after each wrangling step in a super-complex data join and filter.

Thanks! This is surely the most positive peer review comment I've ever had, and made my day! I'm really pleased to hear it is useful.

robchallen commented 1 year ago

Sorry for delays responding to comments. I've fixed everything I can now and thanks to your pointers think the newest version is a big improvement.

All the changes are currently in the main branch under the version 0.2.4.9001. I'll leave the version as is until you've had a chance to look, and if/when you are happy with it I will bump it to 0.2.5 and release it to r-universe and submit update to CRAN.

Thanks again for putting in the effort to review this. I really appreciate it.

robchallen commented 1 year ago

@debruine, I think you may still need to tick one more box on this page if everything OK from your perspective. N.B. I'm about to submit updates to CRAN.

debruine commented 1 year ago

Sorry! That was an oversight. Thanks for the reminder.

ajstewartlang commented 1 year ago

Many thanks for your reviews @debruine and @craig-willis - can I check you're both now happy that all issues have been addressed and for @robchallen to bump the JOSS submission up to 0.2.5?

robchallen commented 1 year ago

(Obviously I found a typo at this point)

debruine commented 1 year ago

I am happy that all issues have been addressed.

craig-willis commented 1 year ago

My concerns have all been addressed. (Thanks for sharing your work @robchallen and @debruine it was great to see your feedback.)

ajstewartlang commented 1 year ago

@robchallen can I check that you've fixed that typo? Can you also confirm you've bumped the JOSS version to 0.2.5? I'll then set that as the branch and do a few final checks. Thanks for a great submission!

robchallen commented 1 year ago

Thanks @ajstewartlang, @craig-willis and @debruine. Really useful to get the comments and feedback and I learnt a lot in the process.

I'm not sure what order to do this all but yes I have fixed the typo(s) in release-0.2.5 branch, I have deployed that version to the R-universe and will now submit as an update to CRAN.

ajstewartlang commented 1 year ago

@editorialbot set release-0.2.5 as branch

editorialbot commented 1 year ago

Done! branch is now release-0.2.5

ajstewartlang commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

ajstewartlang commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1136/bmj.c332 is OK
- 10.1186/s12916-014-0241-z is OK
- 10.1371/journal.pmed.1001885 is OK
- 10.1371/journal.pmed.1001711 is OK
- 10.5121/IJDMS.2011.3207 is OK
- 10.1371/journal.pmed.0040297 is OK
- 10.7326/M18-1376 is OK
- 10.21105/joss.01686 is OK
- 10.1136/bmj.n579 is OK
- 10.2139/ssrn.4087373 is OK
- 10.1002/1097-024X(200009)30:11<1203::AID-SPE338>3.0.CO;2-N is OK
- 10.2218/ijdc.v16i1.763 is OK
- 10.1101/2022.06.29.22277044 is OK
- 10.21105/joss.02959 is OK
- 10.1145/3311955 is OK
- 10.3390/informatics5010012 is OK

MISSING DOIs

- None

INVALID DOIs

- None
ajstewartlang commented 1 year ago

:wave: @robchallen

If you could now do the following please, that would be great:

robchallen commented 1 year ago

10.5281/zenodo.7352369

https://github.com/terminological/dtrackr/releases/tag/0.2.5-cran-submission

@ajstewartlang Many thanks. I think I have done this bit right - it looks all correct on zenodo.

ajstewartlang commented 1 year ago

@editorialbot set v0.2.5 as version

editorialbot commented 1 year ago

Done! version is now v0.2.5

ajstewartlang commented 1 year ago

@editorialbot set 10.5281/zenodo.7352369 as archive

editorialbot commented 1 year ago

Done! Archive is now 10.5281/zenodo.7352369

ajstewartlang commented 1 year ago

@editorialbot recommend-accept

editorialbot commented 1 year ago
Attempting dry run of processing paper acceptance...
editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1136/bmj.c332 is OK
- 10.1186/s12916-014-0241-z is OK
- 10.1371/journal.pmed.1001885 is OK
- 10.1371/journal.pmed.1001711 is OK
- 10.5121/IJDMS.2011.3207 is OK
- 10.1371/journal.pmed.0040297 is OK
- 10.7326/M18-1376 is OK
- 10.21105/joss.01686 is OK
- 10.1136/bmj.n579 is OK
- 10.2139/ssrn.4087373 is OK
- 10.1002/1097-024X(200009)30:11<1203::AID-SPE338>3.0.CO;2-N is OK
- 10.2218/ijdc.v16i1.763 is OK
- 10.1101/2022.06.29.22277044 is OK
- 10.21105/joss.02959 is OK
- 10.1145/3311955 is OK
- 10.3390/informatics5010012 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

:wave: @openjournals/bcm-eics, this paper is ready to be accepted and published.

Check final proof :point_right::page_facing_up: Download article

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3762, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

Kevin-Mattheus-Moerman commented 1 year ago

@editorialbot set 0.2.5-cran-submission as version

editorialbot commented 1 year ago

Done! version is now 0.2.5-cran-submission

Kevin-Mattheus-Moerman commented 1 year ago

@robchallen I am the AEiC for this submission and here to help process this submission for acceptance in JOSS. Below are some minor points that require your attention.

General points:

On the paper:

On the license:

robchallen commented 1 year ago

@Kevin-Mattheus-Moerman Many thanks.

I'm trying to synchronise changes for JOSS and CRAN and tying myself in knots.

On the paper: I'll make those two changes on the paper (UK and biomedical). If I do that and then commit back to the main branch I will then need to do another release which will change the zenodo details....

From my the perspective the "0.2.5-cran-submission" release is fine - however the zenodo archive automatically gets updated whenever I make a github release. I do need to fix a URL for CRAN as well as those two tweaks to the paper shall I fix those and create a new release and give you a new zenodo DOI?

For the license issue, the problem is that CRAN require that the LICENSE file for MIT licensed content contain only the licensor / year and not the full details which must be supplied separately in the LICENSE.md field. Any change to this will result in the CRAN submission getting rejected. if you take a look at another MIT licensed CRAN package (e.g. https://github.com/tidyverse/dplyr/) you'll see this same pattern and GitHub similarly doesn't pick up the license.

Basically I can't change the way the LICENSE files are without breaking the CRAN submission which is insanely particular about what it will accept, and there is no way to simultaneously make GitHub and CRAN happy. Can you live with them the way they are?

Kevin-Mattheus-Moerman commented 1 year ago

From my the perspective the "0.2.5-cran-submission" release is fine - however the zenodo archive automatically gets updated whenever I make a github release. I do need to fix a URL for CRAN as well as those two tweaks to the paper shall I fix those and create a new release and give you a new zenodo DOI?

That sounds good. Sorry this is an awkward process.

For the license issue, the problem is that CRAN require that the LICENSE file for MIT licensed content contain only the licensor / year and not the full details which must be supplied separately in the LICENSE.md field. Any change to this will result in the CRAN submission getting rejected. if you take a look at another MIT licensed CRAN package (e.g. https://github.com/tidyverse/dplyr/) you'll see this same pattern and GitHub similarly doesn't pick up the license.

Basically I can't change the way the LICENSE files are without breaking the CRAN submission which is insanely particular about what it will accept, and there is no way to simultaneously make GitHub and CRAN happy. Can you live with them the way they are?

Okay. Yes, just leave things (in terms of the license stuff) the way they are then.

Kevin-Mattheus-Moerman commented 1 year ago

@robchallen let me know when we are ready to proceed.

Kevin-Mattheus-Moerman commented 1 year ago

@robchallen :wave: let me know when you've made those changes. Thanks.