Skip to content
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

rrlite #6

Closed
7 of 8 tasks
richfitz opened this issue Mar 31, 2015 · 23 comments
Closed
7 of 8 tasks

rrlite #6

richfitz opened this issue Mar 31, 2015 · 23 comments

Comments

@richfitz
Copy link
Member

@richfitz richfitz commented Mar 31, 2015

    1. What does this package do? (explain in 50 words or less)
      This package provides an interface to "rlite" - a standalone, zero-configuration port of the Redis interface (rlite is to Redis what sqlite is to MySQL).
    1. Paste the full DESCRIPTION file inside a code block (bounded by ``` on either end).
Package: rrlite
Title: R bindings to rlite
Version: 0.1.0
Description: R bindings to rlite.  rlite is a "self-contained,
  serverless, zero-configuration, transactional redis-compatible
  database engine. rlite is to Redis what SQLite is to SQL.".
Depends: R (>= 3.1.2)
License: BSD_2_clause + file LICENSE
LazyData: true
Author: Rich FitzJohn
Maintainer: Rich FitzJohn <rich.fitzjohn@gmail.com>
Suggests: testthat,
    knitr,
    RcppRedis
Imports: R6, Rcpp
LinkingTo: Rcpp
VignetteBuilder: knitr
SystemRequirements: GNU make
    1. URL for the package (the development repository, not a stylized html page)
      https://github.com/richfitz/rrlite
    1. What data source(s) does it work with (if applicable)?
      User-supplied, but arbitrary. Special support for data.frames. No direct support for existing remote data sources.
    1. Who is the target audience?
      Package developers, technically minded users who need to process large data or scale analyses. This is a low-level package.
    1. Check the box next to each policy below, confirming that you agree. These are mandatory.
  • The repository has continuous integration with Travis and/or another service
  • The package contains a vignette
  • The package contains a reasonably complete readme with devtools install instructions
  • The package contains unit tests
    1. Do you agree to follow the rOpenSci packaging guidelines? These aren't mandatory, but we strongly suggest you follow them. If you disagree with anything, please explain.
  • Are there any packages that your package depends on that are not on CRAN?
  • Do you intend for this package to go on CRAN?
  • Does the package have a CRAN accepted license?
  • Did devtools::check() produce any errors or warnings? If so paste them below.
* checking for executable files ... WARNING
Found the following executable files:
  src/rlite/deps/lua/src/lua
  src/rlite/deps/lua/src/luac

(this can be fixed by uncommenting the cleanup script but that slows down local development and I've not worked out a good workflow there. Rbuildignore has proved difficult to use for this).

    1. Please add explanations below for any exceptions to the above:
      Not covered in the requested information, but in it's current state, the package is highly unlikely to work under Windows. I'm holding off attempting with R this until the upstream rlite is confirmed to work. Once that's done, this package would definitely want appveyor support to ensure continued Windows compatibility.
@sckott
Copy link
Contributor

@sckott sckott commented Apr 1, 2015

hey @jeroenooms

@richfitz package is our first to go through this process & acts as a test case to work out bugs before we start sending packages through the process. Do you mind reviewing this package? We'll check things that can be automated, including things that Ripley gets pissed about. Started a list of things for reviewers to check in the wiki https://github.com/ropensci/onboarding/wiki/For-Reviewers

@karthik @cboettig @richfitz @jennybc thoughts on the things for reviewers to check in the wiki?

Loading

@karthik
Copy link
Member

@karthik karthik commented Apr 1, 2015

@sckott The list is a good start. The last bit might be hard for a reviewer to judge since we haven't articulated that ourselves in policies. I've been taking a crack at that we should proceed with this one and answer questions as they arise. We can use that to keep iterating on that wiki.

Loading

@jeroen
Copy link
Member

@jeroen jeroen commented Apr 2, 2015

Sure, I'll have a look at it next week.

Loading

@gaborcsardi
Copy link

@gaborcsardi gaborcsardi commented Apr 2, 2015

IMO a lot of these can be automated, so why don't we write a checkr package that does the automated parts, and then shows a nice summary. Parts of R CMD check can be used I guess, although its code quite involved and thus lengthy and somewhat unclear (to me).

Loading

@richfitz
Copy link
Member Author

@richfitz richfitz commented Apr 2, 2015

Ideally, because each package is on travis we can poll to get the most recent build logs? That gives all the R CMD check output without having to rerun anything. I agree that the more that is automated the better.

However, the items on the "for reviewers" checklist are subjective and not easily automatable. The intent here is that new packages to ropensci go beyond just working and are following and evolving set of best practices.

Loading

@karthik
Copy link
Member

@karthik karthik commented Apr 2, 2015

However, the items on the "for reviewers" checklist are subjective and not easily automatable. The intent here is that new packages to ropensci go beyond just working and are following and evolving set of best practices.

💯 to Rich's point. Nearly everything that needs to be automated already has. We need human oversight on fit and code quality.

Loading

@stewid
Copy link
Member

@stewid stewid commented Apr 10, 2015

@richfitz @sckott @karthik @cboettig @jennybc

Could the code be DRYed out? Is there a lot of code duplication?**

No

Are there user interface improvements that could be made?

No, the user interface was clearly described in the vignettes and intuitive to use.

Are there performance improvements that could be made?

No

Is documentation (installation instructions/vignettes/examples/demos) adequate?

The documentation is adequate with installation instructions that are easy to follow, two good vignettes and examples to all methods.

Does the package seem like a good fit for rOpenSci?

Yes 👍 It seems like a very efficient package to work with key-value data.

Other comments:

rrlite and git2r has chosen completly different strategies how to include and update the source code of the underlying interface. It would be interesting to have a follow up and list pros and cons with each approach for future guidance and best practice.

Loading

@richfitz
Copy link
Member Author

@richfitz richfitz commented Apr 10, 2015

Thanks, @stewid! 🌟

With packaging sources - I agree that there are pros and cons (there's a third approach in jqr that seems less robust than either). The git2r compilation also looks really different (https://github.com/ropensci/git2r/blob/master/src/Makevars.in)

Perhaps we could draft a blog post on "including 3rd party software in a package" and include best practices about listing authors, etc?

Loading

@stewid
Copy link
Member

@stewid stewid commented Apr 10, 2015

I'm using the Makefile https://github.com/ropensci/git2r/blob/master/Makefile#L62 to 1) copy files from a local libgit2 repository to git2r, 2) apply patches e.g. to pass R CMD check, and 3) create Makevars.in and Makevars.win from an R script https://github.com/ropensci/git2r/blob/master/scripts/build_Makevars.r

Loading

@karthik
Copy link
Member

@karthik karthik commented Apr 10, 2015

Perhaps we could draft a blog post on "including 3rd party software in a package" and include best practices about listing authors, etc?

💯 👏

Loading

@karthik
Copy link
Member

@karthik karthik commented Apr 12, 2015

@stewid If there isn't anything more for @richfitz to resolve, ok for us to move this into ropensci?
cc @sckott

Loading

@stewid
Copy link
Member

@stewid stewid commented Apr 12, 2015

Yes

Loading

@karthik
Copy link
Member

@karthik karthik commented Apr 12, 2015

ok great.
@richfitz Since you do have ownership of ropensci, go ahead and transfer it when you get a chance.
Other things to check off:

  • Add a rOpenSci footer.
  • Add an appropriate entry into ropensci.org/packages/index.html

Loading

@richfitz
Copy link
Member Author

@richfitz richfitz commented Apr 12, 2015

Sure - I was just going to squash the issues raised over here first.

One thing though; I was thinking about factoring out the auto-generated API (that'd be the "Intermediate" and "High" level interfaces in the rrlite readme) into a new package RedisAPI and leaving rrlite as a driver for rrlite. That would make the interface work more smoothly with RcppRedis and rredis. Would it be OK to transfer both halves into rOpenSci?

Loading

@karthik
Copy link
Member

@karthik karthik commented Apr 12, 2015

Right, missed that. Makes sense to do that first.

Would it be OK to transfer both halves into rOpenSci?

Sounds ok to me. @sckott any objections?

Loading

@jeroen
Copy link
Member

@jeroen jeroen commented Apr 12, 2015

Perhaps you can add a configure script with:

git submodule init
git submodule update

so dependencies will be automatically pulled in if not already? Do you plan to bundle a frozen version of the rlite source code when publishing to CRAN?

Loading

@richfitz
Copy link
Member Author

@richfitz richfitz commented Apr 12, 2015

Yes - I'll grab the current point of the rlite sources for CRAN. Probably once rlite stabilises I'll shift to pointing at its releases (right now it's a moving target).

Loading

@sckott
Copy link
Contributor

@sckott sckott commented Apr 13, 2015

no objections to putting both in ropensci, sounds good

Loading

@richfitz richfitz mentioned this issue Apr 13, 2015
@richfitz
Copy link
Member Author

@richfitz richfitz commented Apr 13, 2015

I've transferred ownership, though there a couple of remaining things in ropensci/rrlite#4

Loading

@richfitz richfitz closed this Apr 13, 2015
@cboettig
Copy link
Member

@cboettig cboettig commented Apr 13, 2015

👏 🚀

Loading

@sckott
Copy link
Contributor

@sckott sckott commented Apr 13, 2015

welcome on 🐗 d

Loading

@karthik
Copy link
Member

@karthik karthik commented Apr 14, 2015

@sckott you're 🔪ing me

Loading

@sckott
Copy link
Contributor

@sckott sckott commented Apr 14, 2015

:p

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants