-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Rclean: A Tool for Writing Cleaner, More Transparent Code #327
Comments
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. |
Thanks! |
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: |
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. |
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. |
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. |
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? |
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! |
Now completed, tested and merged into dev branch. |
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: |
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. |
@annakrystalli FYI - master is now up to date with dev (now v1.1.0). CRAN is still v1.0.0. |
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: ✨ |
Another minor thing I've just noted. In the |
Great! Thanks for the pointers. Per these suggestions, I’ll go ahead and:
|
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
|
Ah, thanks for catching that @wlandau. Must have been from a copy-paste. Will fix now! |
Fixed and committed to master. |
Confirmed, thanks. You have addressed all my feedback, and |
Thanks for your efforts all involved! 👏
I'm currently on the mountain 🏂 on my last day of hols but will set the wheels in motion for finalisation of approval tomorrow morning when I'm back at my laptop.
…Sent from my iPhone
On 8 Jan 2020, at 01:14, Will Landau ***@***.***> wrote:
Confirmed, thanks.
You have addressed all my feedback, and Rclean has come a long way in a short time. As a reviewer, I approve Rclean for rOpenSci. Well done, @MKLau!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Thanks @wlandau, and everyone else for the input/help! @annakrystalli looking forward to it, but have a great rest of the holiday and get safely down off the mountain! |
Approved! 🥳🙌 Thanks @MKLau for submitting and @wlandau and @nevrome for your reviews! To-dos:
For submission to JOSS
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions. We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
Note that this is already under review by JOSS, and that review has been paused while the rOpenSci has proceeded. Once this rOpenSci process is complete, the JOSS review can restart, and should be fast. |
Great, thanks Anna (@annakrystalli), will get on those ASAP. Also, thanks @danielskatz for keeping the manuscript in a holding pattern. It has been greatly improved via the software review. Looking forward to getting it finished. |
Congratulations @MKLau on passing review, and your pending JOSS publication. Would you be interested in writing a Tech Note for rOpenSci about your package? Anna suggests that it is an "interesting package that could really simplify creating reproducible examples and pairing down code to the essentials for producing a given output." Tech Notes are written for an audience that wants details and should provide something a reader could not glean from the documentation itself. If interested, after JOSS publication is probably best timing, so you could respond then. Instructions are here: https://github.com/ropensci/roweb2#contributing-a-blog-post. Typically you would submit a draft by pull request, I review, and we can publish within a week. |
Thanks, @stefaniebutland, I have read a number of the Tech Notes you’ve helped to put out, and I’ve always have found them to be engaging and useful. I’m definitely interested and I’ll keep it in mind as the manuscript is in review. |
Hi @annakrystalli, Almost done with the transfer to ROS and edits on the JOSS manuscript. Github has been throwing a slow Unicorn! message whenever I try and create a pull request now that Rclean has moved to ROpenSci. Will try again tomorrow to merge the new changes that have updates to Travis and Codecov. A couple questions:
Thanks! Matt |
Hello @MKLau ! I've just transferred full admin rights back to you so you should have full control of the repo again. The paper looks good to me! I wonder if you should show loading the library? Otherwised I reckon it's good to go. And yes well spotted, please ignore initial instruction to |
Thanks for looking over the paper, @annakrystalli . I'll go ahead and add a line showing library("Rclean"). One more question, how do I enable Zenodo watching? I don't see it when I access Zenodo. Should I have done this prior to transferring the repo to ROpenSci? Also, I keep getting a long loading time page. Do you have access to the repo? Would you be able to check if you can open a new pull request? |
I'm going to go ahead and send to JOSS. Let me know if you get a chance to look at the pull request initiation from your end though. |
Related to ropensci/software-review#327
Hi @MKLau, just looking into the pull request issues you're having. I've managed to make a successful PR (not merged). ropensci-archive/Rclean#201 When and where exactly are you getting the slow unicorn message? |
Ah, looks like it's resolved now! I can see your pull request. It was throwing the slow unicorn every time I tried to go to view the pull request. Maybe it had something to do with the transfer from my personal profile to the ROS org. |
Good to hear it's all working now! I take it you are in the process of completing with JOSS too right? I'm going to go ahead and close this issue now. |
@annakrystalli yes it's now underway (again). Thanks again for all your help! |
Congratulations on your JOSS publication @MKLau. We would love to have a technote about Rclean as noted above
We now have more detailed guidance: https://blogguide.ropensci.org/ If you're interested, please suggest a date for submission and I can provide a publication date. |
Hi @stefaniebutland, yes, we would be interested in publishing a technote with an announcement of the package. I am currently in a grant writing period but would be able to write something up next week. Would that time frame work? |
Yes it would thank you. Please submit when you're ready using tentative publication date 2020-03-17. cc @ropensci/blog-editors |
Great, sounds good. |
Hi @stefaniebutland, Just finished with proof reading and spellcheck of the technote. You can find it here: https://github.com/ropensci/Rclean/blob/technote/ropensci/rclean_technote.md The associated Rmd file is in the same directory on that branch too, if you need to have a look. I’ve written it as an expanded version of the JOSS article, with a bit more discussion of a few details. Happy to have any edits and/or suggestions though. Thanks and hope you’re healthy and well. |
Hi @MKLau, I've been chatting with @stefaniebutland and we were thinking that your article looks a bit more like a blog post than a tech note (particularly the section talking about the Provenance Engine). This is great! To get this published, I invite you to submit it as a pull request to the I'll be your friendly reviewer and will be ready to review on Monday, if you can open the pull request by then. Once you're ready for review, either let me know in a comment on the pull request or change the pull request from Draft to Non-Draft. Thanks! |
Hi @steffilazerte, thanks for enlisting to review! Happy to go either way. If this seems more like a blog post to you two, I'm fine with that. I'll read up on roweb2 and I'll aim to submit before Monday morning. |
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.
The text was updated successfully, but these errors were encountered: