-
-
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
ezknitr package #56
Comments
Thanks for the submission, @daattali. I'll be your handling editor. I'll do editor's checks this week before assigning reviewers. |
I'm happy to review. |
Hi @daattali, could you comment on Question 6 above re: overlapping packages? As you note in the README, there's some feature overlap with rmarkdown. Also rprojroot provides an alternative solution for the working directory issue. Neither of these preclude acceptance of ezknitr, but we'd like your take on the advantages/disadvantages of your approach and which use-cases it is most appropriate for, which will help reviewers give feedback. |
@noamross you're absolutely right, I edited Q6 |
Thanks @daattali. Seeking a second reviewer. Everyone is at UseR! So it may take slightly longer ;) |
Package Review and CommentsOverall this package looks really good. It solves an important limitation of the Please 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:
Documentation comments
Functionality
Functionality comments
Future functionality ideas:
Estimated hours spent reviewing: 4 hrs |
@karthik |
@sckott cool bot! What service are you using? |
@daattali some bespoke ruby https://github.com/ropenscilabs/heythere/ |
@karthik |
Can I retroactively add the "automatic publishing to JOSS" info? |
I think so, sound okay @noamross |
I have updated the initial post in this thread and added an 11th bullet point mentioning that I want to submit to JOSS. Perhaps @ttimbers and @karthik can take a quick look at that and give their opinion on that as well? I believe it should take no longer than a few minutes. Thanks! |
Just getting back from vacation, will attend to this in the next few days. |
No rush, still waiting for Karthik for his review till I respond to yours:)
|
The package includes all the following forms of documentation:
Paper (for packages co-submitting to JOSS)The package contains a
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 2.1 Review of ezknitrThis is an extremely useful addition to the Comments
I was going to suggest having a few example use cases, such as a simple report with one level nesting, a complex report (or multiple reports) with data in various places. But the
The function itself does not check to see if these variables have been defined in a document. If I understand correctly, you are asking the user to use set_default_params <- function(params) {
params <- as.list(params)
env <- parent.frame(1)
invisible(
lapply(names(params), function(key) {
if (!exists(key, envir = env, inherits = FALSE)) {
# ^ Is this not supposed to work outside of ezknit?
assign(key, params[[key]], envir = env, inherits = FALSE)
}
})
)
} Also, rather than point out that All other functionality looks as expected. Other suggestions for improvement
This came off as a bit confusing to me at first. Perhaps you could say project root to keep it more general?
Perhaps you can say why this might be useful to do. E.g. if someone is doing this from a makefile or another build script, this could be the way to not worry about where that file resides (and
When reading the README, it is not clear if the variable DATASET_NAME would be set in the yml or the knitr document itself. It is the latter, but it would be good to spell out and perhaps even include a little code snippet.
Suggest linking out to Yihui's page in case such a person is reading. Review of the JOSS paperCould you switch the first two sentences with a bit of a rewrite? Start of with why knitr has been so useful in the R community and what it has enabled? Then point out shortcomings and what ezknitr enables? typo: adresses Once this is fixed, I will check off the last approval box. Overall commentsI think this is a nice addition to the rOpenSci suite and recommend acceptance. 💯🚀 |
Thanks @karthik! Could you prepend the reviewer checklist to On Fri, Aug 12, 2016, 5:12 PM Karthik Ram notifications@github.com wrote:
|
Sorry, I just noticed that I forgot to add all my comments (I had them sitting in a text file -- it 'd be awesome if GitHub supported "saving drafts" of comments). I added/improved everything that was suggested, but here are some comments about things that I didn't change: @ttimbersI thought about adding a cleanup function but decided against it because depending on what function the user runs, files and folders will get generated in different places and they can run any arbitrary command so I can't know what exactly to clean without making a lot of assumptions browseVignette: I think if you install the package from CRAN or GitHub then you will see the vignettes, it's only when you install using devtools that the vignettes don't get built/linked r-lib/devtools#931 README.Rmd - I chose to stick with just a direct README.md because I don't have code chunks. If in the future I'll have R code chunks that will benefit from Rmd -> md then I'll change it out_suffix: if you try to assign it an NA value , then the function will fail with an informative error message ("Error: open_output_dir has no test because all it does is a side effect that is (impossible? super extremely hard?) to test. I don't know of any way to test that a function caused the OS file explorer to open. set_default_params() does have tests https://github.com/daattali/ezknitr/blob/master/tests/testthat/test-set_default_params.R compiling to PDF: as far as I know, there isn't any special function to do that, right? You just need to use pandoc? I'm open to a pull request or an issue being opened but I don't have any experience making PDF so I prefer to leave it as a future improvement. I haven't actually played around with notebooks too much, but I don't think it will be able to interact with notebooks because notebooks seem like they use knitr under the hood (I think - I don't actually know its internals) which means they'll still have the usual work directory problems @karthikset_default_params: the user does not need to make any checks themselves. All the user does is pass in a named list, and every key in the list will be treated as a variable name. Every variable name that currently doesn't exist will get created and assigned the value from the list. The function doesn't need to explicitly check for existence using "exists()" because I do use exists() inside the loop structure to see if an incoming variable exists or not. set_default_params WILL work in any context, not just in ezknitr. you can try it yourself: assign 'x <- 5', then call 'set_default_params(list(x=10, y=20))'. Hopefully x will remain untouched but y will be 20 PS. SO COOL that you can have a little Details bar that collapses/expands on click! |
@daattali, my comments to your comments:
I know that these calls are using an older version of the
|
Review of the JOSS paperLooks good to me! I recommend ROpenSci accept the paper and the package. 😃 |
Using a plain README.md is fine if there are no code chunks. I've just clarified the language in the packaging guide |
Approved! This will be our first JOSS pass-through package to reach this stage of the process. Allow me to pre-apologize for any snafus along the way. Here are the remaining steps, @daattali:
|
I think the behaviour you're seeing from both Re: knitting to PDF: I would happily add support for ezknit to pdf, but does knitr::knit have support for that in the first place? I never do PDF myself so I don't know how people do it. I tried googling it a bit and as far as I can tell, knitr does not have a way to convert to PDF, only Can you let me know if you do know of a way to convert to PDF using knitr? If not, then I'll treat this package as done for now and will go through with the next steps |
Re:
Re: knitting to PDF I guess RStudio had fooled me into thinking the "Knit PDF" button uses a function from the |
Re: Re: knitting to PDF It looks like in theory, you could use Although I must say, I tried this strategy and it didn't work as I ran into an error with Anyways, its a bit frustrating that it is not obvious to if this is what the "Knit PDF" button is doing, and if not why its says "Knit" if it is not using |
Ohh I see what you mean about vignettes! I was under the impression you were trying to call Yes the button is a bit misleading, I'm pretty sure it uses rmarkdown::render... |
@noamross I can't transfer the repo to rOpenSci, it tells me I need admin right Should I update all links referring to |
Oh, it should be ropenscilabs you transfer to. On Tue, Sep 13, 2016, 9:14 PM Dean Attali notifications@github.com wrote:
|
I was just invited to ropenscilabs. Am I supposed to transfer ownership to ropenscilabs or to ropensci? If it's the former, then will it eventually move to ropensci? Does that mean I should update all the links now to point to github.com/ropenscilabs/ezknitr and later on I'll have to update again to point to ropensci? |
yes, but everything autoforwards |
Sorry if this info is available elsewhere, I couldn't find it anywhere. Do packages move from ropenscilabs to ropensci in a matter of hours/days, or do they remain in labs for a long time? (So that I can maybe just skip the ropensci labs URL if it's only temporary) |
They remain for a long time. Honestly there's little difference between the two orgs. We anticipated at one point there being a distinction based on age/stability of packages but that process hasn't really evolved and so we haven't documented it properly ([adds to todo list]). Thankfully all the URLs do forward so when it's transfered (or we re-consolidate) the links will work. On Tue, Sep 13, 2016, 10:21 PM Dean Attali notifications@github.com wrote:
|
I'll do a CRAN submission and when the new version gets on CRAN I'll continue with the transfer process |
Not a matter of hours. After the project grew and the main account was getting cluttered with mature projects and various new/half-finished/non-software repos, we started ropenscilabs. All new projects started by the rOpenSci team begin there, and all accepted projects from the community (like yours) get transferred from a personal account to there. Like @noamross said, GitHub does a great job forwarding repos as they move around. We'll update the |
I just transferred the repo and have a few comments and a question:
|
@noamross in the JOSS submission, for the field of "Repository address", do I enter the github repo or the figshare url? |
Thanks! We're collecting the notes from these little confusions for our first few JOSS submissions. |
Submitted to JOSS. I think I'm all done on my part, or is there anything else to do? |
Nope! All done. I'll close this issue and keep you appraised. Expect some notifications from the JOSS side. |
yeah, I think we need to manually turn on builds |
Allows you to use 'knitr' with complex directory structures and gives you more control over where inputs/outputs/figures/scripts live when knitting documents. Real research projects often don't have a flat directory structure, which can make using 'knitr' on such projects very cumbersome.
https://github.com/daattali/ezknitr
N/A
Any researcher or grad student who wants to better automate and organize their research using
knitr
but find it annoying/impossible to deal with directories in a sane wayezknitr
offers is the ability to pass parameters to Rmd files. There is a difference between the two: rmarkdown assumes that the parameters are defined in the Rmd YAML, while ezknitr lets you change any arbitrary variable. This difference is not too significant, but the main reason ezknitr has this feature is to support older Rmd files that don't have YAML because the introduction of parameterized Rmd files is fairly newrprojroot
helps you find a specific file in relation to a project's root directory, while ezknitr is specifically for dealing with knitting documents in complex directory structuresR CMD check
(ordevtools::check()
) produce any errors or warnings? If so paste them below.Yes, I will follow those guidelines. The package is already on CRAN.
N/A
N/A
The package contains a
paper.md
with a high-level description.The package is deposited in a long-term repository with the DOI:
The text was updated successfully, but these errors were encountered: