-
-
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
RefManageR #119
Comments
Editor checks:
Editor commentsThank you for your submission, @mwmclean! We were actually in the midst of revisions of our Aims and Scope when you submitted this, and one of our updates was to explicitly add reference management as a component of our reproducibility category. Below are the results of our automated tests. I don't see anything here to prevent review going forward, but we will want these addressed before acceptance. This also provides a reference for reviewers of areas they may want to pay attention to. If you make changes in response to these or have a written response, please note them on the thread here so reviewers know about updates. I am currently seeking reviewers.
Running your tests, I get the following messages, plus several more of the locale-based warnings.
In addition,
|
Thanks for the comments. Regarding the last two notes from |
sorry about that @mwmclean I'll have a look |
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:
No CONTRIBUTING document or section of the Readme.
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 3 Review CommentsOverview: The RefManageR package is overall a well designed and carefully engineered package that already sees significant use as indicated by the download statistics. (Indeed, I've used the package myself both as a dependency and for occasional use. The source code is easily readible, well documented, and has recently migrated to the The package aims to provide functions that fall into at least three separate buckets, which from the perspective of a developer, might have made more sense for these to be separated out into more complete, separate module packages. That could streamline maintenance and make it easier for other developers to just depend on the parts they need. More importantly, it could help the package provide somewhat more complete implementations of these separate areas, and enforce a bit better definition of a low-level interface that a developer could use to leverage any part. This will make more sense in terms of specifics, which I discuss below. However, I suspect breaking up the package is beyond the scope here and should not prevent onboarding, but merely as a suggestion to the author and something to keep in mind for future directions. The separate modules, as I see them, are:
Unfortunately, I think separating out these elements also highlights some possible limitations of this design:
To me, these seem a bit like missed opportunities in the design. The core "RefManageR" features in section 2 are excellent, but are restricted to only manipulating data in the biblatex format, and only accessing remote data in the bibtex format, which diminishes the potential somewhat. The lack of more modular abstraction also encumbers the source code a little: for instance, this package makes no use of the existing (ropensci) rcrossref implementation, while it's native implementation makes rather heavy use of |
Thank you, @cboettig, for the quick and helpful report. I will respond more completely to your report very soon, but for now just wanted to mention a couple updates and sticking points.
|
@mwmclean I'm about to upload |
Hi @AmeliaMN, a friendly reminder that your review is due in one week. |
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:
The images in the
I noticed a possible spelling error in the title for the documentation on
The example for When I edited the code to include
The example for
The example for
and part:
I didn't see examples for
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 3.5 Review CommentsThis is my first time doing a package review for rOpenSci, but I think this package looks great. RefManageR provides a variety of helpful functions for managing and citing bibliographic entries in RMarkdown. I was not previously aware of the package, but I will certainly be using it in my own work going forward. All the automated checks passed on my system, and the documentation is clear with thorough examples. As noted above, I ran in to a few warnings I didn't understand, but no errors or bugs. |
Thank you for the helpful comments @AmeliaMN. I think I've managed to address the issues you raised with some commits today up to this one.
|
Thanks for your reviews @cboettig and @AmeliaMN, and your follow-ups @mwmclean. Let me just jump to ask @mwmclean to please let us know when you think your updates have addressed the all the reviewer comments thus far (including commenting here on big-picture stuff). Then we can take a second look at all the changes in aggregate. Also, your |
In response to the Regarding the comments of @cboettig
@noamross, let me know if I've missed anything, but I believe I've now addressed all the comments thus far. |
Yup, 👍 , I think this is ready to go and I think i've checked all my boxes above. |
Looking good! I'm still confused about the Entrez/E-Utilities thing, but I think that's just my lack of familiarity with NCBI. Thanks for adding those additional examples. The only thing I can see now is the example for
|
I believe it's in the help file for
|
Perfect! I've checked off all my boxes. |
Approved! Thanks @mwmclean for submitting and @cboettig and @AmeliaMN for your reviews! To-dos:
Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). Let us know if you are interested, and if so @stefaniebutland will be in touch about it when she's back from vacation late next week. |
@noamross do you still need to turn on coverage services for the transferred repo? I've tried setting up both Codecov and Coveralls with no luck. |
There's an issue with permissions in the transfer to Codecov that we're figuring out that has held up the past couple of migrations. Working on it! Right now it looks like your .travis.yml sends to Codecov (which is not running), but your current badge is pointing to Coveralls. I'll let you know when when have the codecov migration working. |
I switched back to Coveralls. Not sure how long it takes to update, but coveralls.io is still saying "this repo has not received any jobs yet". |
hi @mwmclean - coveralls is turned on now for this repo, can you try again? |
@sckott I restarted a build on travis just after I saw your message. Still saying no builds on coveralls.io. |
So i'm not that familiar with coveralls for R pkgs. It's turned on for your repo, but do you need a Coveralls key on Travis as an env var? Used to need one for codecov, but now just connects automatically. |
Looks like something weird with my package or |
don't know @jimhester ? any thoughts?, see https://travis-ci.org/ropensci/RefManageR/builds/268404448#L2305 |
To debug this you should run Rscript -e 'covr::package_coverage(type = "all", quiet = FALSE, clean = FALSE)' Then you can inspect the package and libraries that are created locally. When I do this I get the following output.
|
Hello @mwmclean. Following up to ask if you are interested in writing a blog post about your package: either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). |
Hi @stefaniebutland, it doesn't look like I'm going to be able to do a blog post in the short term, but I'd like to maybe do one at some point in the future. |
Just ping me if and when you're interested and have the time @mwmclean. Thank you for submitting your package! |
can this be closed? |
ok with me to close re: blog post. @mwmclean please ping me if and when you want to do one |
Sorry, did we need my input on the closing question? I still need to track down my issue with |
Summary
Provides tools for importing and working with bibliographic references. Greatly enhances the bibentry class in R by providing a class BibEntry which stores BibTeX and BibLaTeX references, and can be easily searched by any field, by date ranges, and by various formats for name lists using R's person class.
https://github.com/mwmclean/RefManageR
Reproducibility because it provides functions as well as vignette examples of working together with knitr to include references in reproducible reports. Data access because of the functions it provides for importing bibliographic information into R.
Anyone that wants to write reproducible reports or simply organize bibliographic data from a variety of sources.
No package I'm aware of provides all the functionality, but there is some overlap with other packages.
Base R provides the
bibentry
class.RefManageR
provides a classBibEntry
that extendsbibentry
in a number of ways, such as support forBibLaTeX
; improved handling of name list fields (author, editor, etc.); improved handling of dates (usinglubridate
); additional options for sorting, formatting, and printing; and additional methods that make it easier to search, sort, and modify parts of theBibEntry
object. There is a small overlap with the packagesrentrez
andrcrossref
, in thatRefManageR
can make use of the same APIs that those packages use to get references. However,RefManageR
parses the results differently and returns aBibEntry
object.Requirements
Aside from the packages and version of
R
listed above in the DESCRIPTION, poppler is needed if a user want to extract bibliographic information from PDF files on their machine.Confirm each of the following by checking the box. This package:
Publication options
paper.md
with a high-level description in the package root or ininst/
.Note: I already submitted to JOSS here
Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
Exceptions: I did not follow your guidelines when naming the functions in my package since they were named long ago and have been on CRAN for a long time now (I used title case and no underscores).
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
@karthik @cboettig @sckott
The text was updated successfully, but these errors were encountered: