Track tasks and feature requests
Join 40 million developers who use GitHub issues to help identify, assign, and keep track of the features and bug fixes your projects need.
Sign up for free See pricing for teams and enterprisesRclean: A Tool for Writing Cleaner, More Transparent Code #327
Comments
This comment has been minimized.
This comment has been minimized.
Thanks for your submission @MKLau! I'll be handling your review. Stay tuned while I complete editors checks. I'll get back in touch when they are done. |
This comment has been minimized.
This comment has been minimized.
Thanks! |
This comment has been minimized.
This comment has been minimized.
Editor checks:
Editor commentsInstallation:Hello again @MKLau I'm having issues installing the package. Although installing from CRAN works,
So after adding:
to
Both Adding:
just above I tried just adding Finally, I also got these warnings:
Which suggest that you need to set a minimum version of R in
in your So until these installation issues are resolved, I can't really proceed with the rest of the editors checks as they are all performed on the source code which needs to be able to be installed from source (ie not just downloading the binary from CRAN. For example, running Just let me know when this problem is fixed and I can carry on with checks. Reviewers: |
This comment has been minimized.
This comment has been minimized.
Hi @annakrystalli, thanks for your feedback on this. I've been looking into the issues with installing those dependencies. The package installs from dev on github via devtools on my machine and on Travis' server. However, I manually installed the CodeDepends on my machine, and Travis manually installs as well. I haven't yet done a full install on a fresh machine just using devtools. This is an important issue, as it would make install much easier to just run one command. So, I'll sort this out and let you know. |
This comment has been minimized.
This comment has been minimized.
Hi @annakrystalli, alright, I did a fresh install of R on my machine with the addition of all your dependency fixes to the DESCRIPTION and I get a similar error as you:
Travis still installs without error, which is installing the graph package separately. I've done some searching on Stack Exchange, and I can see why RGraphviz would install but not graph. I'll keep searching for possible workarounds. |
This comment has been minimized.
This comment has been minimized.
Hi Anna, I still haven’t found a solution to the graph package install other than manually installing graph via BiocManager. So, if you’d like you should be able to proceed by first installing graph before installing
It looks like the CodeDepends maintainers are working on a solution for this (duncantl/CodeDepends#30) but currently they’re essentially using the manual install method like this. I’m adding this approach to the dev branch install instructions but I’m dissatisfied with this as an install method. I’ll keep looking for a fix and follow the updates from the CodeDepends folks. |
This comment has been minimized.
This comment has been minimized.
Hey @MKLau, sorry for slow reply, I am actually on holiday so will probably be slow getting back to you for the next week also. Apologies! In the meantime, I found a short term workaround that hopefully we might be able to turn into a more long-term one. I forked I also changed the remote repository pointer in the I'll make a pull request to the Sound good? |
This comment has been minimized.
This comment has been minimized.
Great, @annakrystalli! Copy that. I’ll use this workaround and follow what the CodeDepends issue you opened. Hopefully they just merge it in, which seems likely. Enjoy the rest of your break! |
This comment has been minimized.
This comment has been minimized.
Now completed, tested and merged into dev branch. |
This comment has been minimized.
This comment has been minimized.
OK great and apologies for massive delay! Been pretty snowed under since I got back and still catching up. I've now been able to complete editors checks and installation worked fine with the new setup. So, firstly and most importantly, I'd recommend that all your installs happen from the The rest of the editors checks threw up only minor issues. goodpracticegoodpractice::gp() Throws a warning
I wonder, are there calls to functions in spell_checkA few potential genuine typos devtools::spell_check("/Users/Anna/Documents/workflows/rOpenSci/editorials/Rclean")
#> WORD FOUND IN
#> formate p.spine.Rd:10
#> funcitons codeGraph.Rd:16
#> inforrmation get.libs.Rd:16
#> parantage get.spine.Rd:16
#> Parenatage p.spine.Rd:5
#> Prases parse.graph.Rd:5
#> reltionships parse.graph.Rd:6
#> reproduciblity README.md:49
#> Rclean.rmd:41 Created on 2019-09-10 by the reprex package (v0.3.0) package coverageGreat coverage overall! Is there are reason covr::package_coverage("/Users/Anna/Documents/workflows/rOpenSci/editorials/Rclean")
#> Rclean Coverage: 81.22%
#> R/var.id.R: 0.00%
#> R/write.code.R: 45.45%
#> R/var.lineage.R: 70.00%
#> R/p.spine.R: 72.73%
#> R/parse.graph.R: 81.82%
#> R/parse.info.R: 87.50%
#> R/clean.R: 91.67%
#> R/codeGraph.R: 100.00%
#> R/get.libs.R: 100.00%
#> R/get.spine.R: 100.00%
#> R/read.prov.R: 100.00% Created on 2019-09-10 by the reprex package (v0.3.0) @MKLau, all in all I'm happy to proceed with finding reviewers. If you could have a go at fixing these minor issues during that time, that would be great. For me the most important is to get the master branch up to date and use that as the stable version of your package that will be reviewed. Could you please add the rOpenSci under review badge to your README?
Thanks again for your patience during the holidays! Reviewers: |
This comment has been minimized.
This comment has been minimized.
Hi @annakrystalli, no worries! Hope you had a relaxing/restorative holiday.
Thanks for the updates on this, and happy to see the review move forward. |
This comment has been minimized.
This comment has been minimized.
@annakrystalli FYI - master is now up to date with dev (now v1.1.0). CRAN is still v1.0.0. |
This comment has been minimized.
This comment has been minimized.
Great work @MKLau ! Regarding your comments:
You should treat the GitHub version of your code as the development version in general and it is absolutely fine for it to not reflect CRAN. The way you link your GitHub code version to the CRAN released it through tags which snapshot more formally the state of the repository upon release. After that you are free to continue developing on GitHub by adding That is somewhat different to the use of branches. You still want the
My go-to strategy in these situations is to have a look at the tests in packages that use the feature I want to test. I initially thought of For all your other comments: |
This comment has been minimized.
This comment has been minimized.
Another minor thing I've just noted. In the |
This comment has been minimized.
This comment has been minimized.
Great! Thanks for the pointers. Per these suggestions, I’ll go ahead and:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 3
Review CommentsA very interesting package to simplify R scripts. I really like the underlying idea and the main functions
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@annakrystalli, that’s what I was planning. Double thanks @nevrome! |
This comment has been minimized.
This comment has been minimized.
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 4
Review Comments
Documentation
API
Installation and automated checks
install.packages("BiocManager")
BiocManager::install(c("graph", "Rgraphviz"))
It is good practice to
✖ write unit tests for all functions, and all package code
in general. 86% of code lines are covered by test cases.
R/clean.R:49:NA
R/clean.R:72:NA
R/clean.R:82:NA
R/clean.R:91:NA
R/clean.R:95:NA
... and 21 more lines
✖ use '<-' for assignment instead of '='. '<-' is the
standard, and R users and developers are used it and it is easier
to read your code for them if you use '<-'.
R/var.id.R:41:44
R/var.id.R:42:47
✖ 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/clean.R:47:1
R/clean.R:93:1
R/get.spine.R:44:1
R/parse.info.R:39:1
R/parse.info.R:40:1
... and 1 more lines
✖ avoid sapply(), it is not type safe. It might return a
vector, or a list, depending on the input data. Consider using
vapply() instead.
R/clean.R:150:29
R/clean.R:151:55
R/clean.R:169:34
✖ 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.libs.R:40:13
R/get.libs.R:42:17
R/get.libs.R:44:21
R/var.id.R:39:13
R/var.lineage.R:43:22
... and 4 more lines Miscellaneous
|
This comment has been minimized.
This comment has been minimized.
@wlandau thanks so much for a speedy review as well! Thanks to everyone for helping to improve and push this project forward. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
One clarification on a couple points I made:
Those functions are necessary to implement In fact (and this is not strictly necessary to satisfy my review) it could be helpful to split
|
This comment has been minimized.
This comment has been minimized.
Thanks for taking the time to clarify @wlandau. I get what you’re saying both about the structure of the API and the logic of the Also, thanks for the pointers, especially the cyclocomp package. |
This comment has been minimized.
This comment has been minimized.
Please update openjournals/joss-reviews#1312 when this is accepted. |
This comment has been minimized.
This comment has been minimized.
Hi @MKLau, Just checking in with you. Do you have an ETA for a response to reviewers? |
This comment has been minimized.
This comment has been minimized.
Hi @annakrystalli, I’m finishing up addressing a final comments this week, and, as long as those get done, I should be able to write-up a response early next week. |
This comment has been minimized.
This comment has been minimized.
General ResponseThanks again to @wlandau and @nevrome for reviewing the package and, Response to Reviewer CommentsDocumentation
JOSS Co-Submission@wlandau: missing all sections
@MKLau: The initial submission of the JOSS paper was based on an FunctionalityAPI
Installation and automated checks
Miscellaneous
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@MKLau, fantastic work on the revisions! You put a ton of effort into refactoring the code, rethinking the package scope, and strengthening the documentation. The behavior of After reviewing MKLau/Rclean@b674396, I have some minor follow-up comments and requests.
Rclean Coverage: 89.69%
R/keep.R: 83.33%
R/clean.R: 85.00%
R/get_libs.R: 85.71%
R/get_vars.R: 85.71%
R/code_graph.R: 92.31%
R/var_lineage.R: 100.00%
filename functions line value
51 R/clean.R clean 57 0
53 R/clean.R clean 58 0
79 R/clean.R get_path 132 0
81 R/clean.R get_path 135 0
82 R/clean.R get_path 136 0
83 R/clean.R get_path 137 0
20 R/code_graph.R code_graph 46 0
9 R/get_libs.R get_libs 42 0
18 R/get_vars.R get_vars 45 0
31 R/keep.R keep 49 0
✖ 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/code_graph.R:15:1
R/keep.R:17:1
R/var_lineage.R:15:1
R/var_lineage.R:22:1
✖ fix this R CMD check NOTE: Namespace in Imports field not
imported from: ‘jsonlite’ All declared Imports should be used. Without retrospective provenance, it appears you no longer need
#' data(simple_script)
#' ## Copies code to your clipboard
#' keep(simple_script)
#' ## Saves code to your disk
#' keep(simple_script, file = "clean_code.R") |
This comment has been minimized.
This comment has been minimized.
I agree with @wlandau: You did an excellent job refactoring Rclean, @MKLau. I really enjoyed reading the vignette and running the example code. I will keep this in mind to analyse my own scripts. Maybe an RStudio Addin would be nice: Select a variable in your code file with your mouse, press a key combo, get the Some last, small remarks:
with
If you want to add the reviewers to the DESCRIPTION file, then you can use the following string for me:
|
This comment has been minimized.
This comment has been minimized.
pinging @labarba as the JOSS editor for that part of this - note the comment about the JOSS paper and review (openjournals/joss-reviews#1312) in the comment above. |
This comment has been minimized.
This comment has been minimized.
@danielskatz — I don't understand what I need to opine about. |
This comment has been minimized.
This comment has been minimized.
the part:
|
This comment has been minimized.
This comment has been minimized.
When a paper goes through rOpenSci review, it gets handled by @arfon on the JOSS side. I have not handled any of these. |
This comment has been minimized.
This comment has been minimized.
Actually, it can be handled by the AEiC on duty - I've done this a few times, as have others. See https://joss.readthedocs.io/en/latest/editing.html#processing-of-ropensci-reviewed-and-accepted-submissions |
This comment has been minimized.
This comment has been minimized.
In any event, to answer @nevrome's question, my understanding is that once this submission is accepted by rOpenSci, the JOSS Associate-Editor-in-Chief responsible for the JOSS review (which is currently @labarba) will review the paper to make sure it meets JOSS standards, but will assume the review of everything else done by rOpenSci is sufficient. (But maybe it's worth asking @arfon to make sure I have this right) |
This comment has been minimized.
This comment has been minimized.
Thank you for your answer, @danielskatz. My issue is mostly caused by the unclear border between code and text review. @wlandau and I provided code review according to this Guide for Reviewers. The guide does not mention the JOSS paper. The review template contains the following section, though:
So obviously it is expected that I read and review the text. Also from my understanding of your answer, @danielskatz, there won't be additional review beyond a rather general check of the editor over at JOSS. The takeaway message is that the course of action suggested above by @MKLau is not possible:
If there are significant changes of the article AFTER this review, then they won't be reviewed at all. |
This comment has been minimized.
This comment has been minimized.
Thanks @nevrome for picking this up. What we can do is consider the code review of the package complete and move it to rOpenSci. I will coordinate with @MKLau and when the |
This comment has been minimized.
This comment has been minimized.
Well, they are reviewed by the Associate-Editor-in-Chief, but we would certainly appreciate anything you do that improves the paper. Whether this is an obligation of rOpenSci reviewers of submissions that are also being submitted to JOSS is for rOpenSci to determine. |
This comment has been minimized.
This comment has been minimized.
Thanks for the clarification, @annakrystalli and @danielskatz. The solution suggested by you, @annakrystalli, is fine for me. I can also offer to read the text when it's ready and if you have the feeling that some additional feedback might be helpful, @MKLau. |
This comment has been minimized.
This comment has been minimized.
Great, thanks to everyone for clarifying the relationship between the software and article review. Thanks for the offer to review the manuscript (@nevrome), it would be great to get your input on it if you have the time to spare. @annakrystalli, I’ll send you the updated manuscript soon. Hopefully by week’s end. |
This comment has been minimized.
This comment has been minimized.
@wlandau and @nevrome, thanks also for the follow-up comments and suggestions.
Thanks! |
MKLau commentedJul 23, 2019
•
edited
Submitting Author: Matthew K. Lau (@MKLau)
Repository: https://github.com/MKLau/rclean
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below.:
Explain how the and why the package falls under these categories (briefly, 1-2 sentences). Please note any areas you are unsure of:
In writing analytical scripts, software best practices are often a
lower priority than producing inferential results, leading to large,
complicated code bases that often need refactoring. The "code
cleaning" capabilities of the Rclean package provide a means to
rigorously identify the minimal code required to produce a given
result (e.g. object, table, plot, etc.), reducing the effort required
to create simpler, more transparent code that is easier to reproduce.
this package?
The target audience is domain scientists that have little to no formal
training in software engineering. Multiple studies on scientific
reproducibility have pointed to data and software availability as
limiting factors. This tool will provide an easy to use tool for
writing cleaner analytical code.
how does yours differ or meet our criteria for
best-in-category?
There are other packages that analyze the syntax and structure of
code, such as lintr, formatr and cleanr. Rclean, as far as we are
aware, is the only package written for R that uses a data provenance
approach to construct the interdependencies of objects and functions
and then uses graph analytics to rigorously determine the desired
pathways to determine the minimal code-base needed to generate an
result.
Not that I can think of at the moment.