search

r-lib / R6

Encapsulated object-oriented programming for R
https://R6.r-lib.org
Other
407 stars 56 forks source link

Basic support to mimick the use of interface classes #81

Closed jankowtf closed 7 years ago

jankowtf commented 8 years ago

Hi Winston,

I kindly ask you to consider accepting this pull request. It is associated with my addition to #9 yesterday.

I figured that I actually don't really need multiple inheritance but just a way to distinguish between

  1. "interface classes" to mimicking interfaces/abstract classes. These only define public abstract methods that a concrete class needs to implement, so a simple "check if method xy is implementend in $new() would really kind of suffice.
  2. "concrete classes" for actual inheritance

I'm aware that I might be pushing R6's intended use and R's class inheritance mechanism a bit, but I personally like the SOLID principles in general and that of interfaces in particular very much. I think following those simply makes for better code and IMO even "just" prototyping OO stuff in R with R6 shoudn't be an exception.

This is my first substantial pull request for another person's codebase. I tried to be both stick to your style and be thorough (updated doc and unit tests), but probably something still slipped through the cracks ;-) So please don't hesitate to contact me in case there's something else that I can do!

Note: I noticed that my modification of the print method is a bit of in cases where get(class(x)[1]) fails in getImplementClassname (script R/interface_utils.R). It worked upon manual execution of test_that(), but not when the file was sourced.

jankowtf commented 8 years ago

Hi Winston,

it seems like you don't particularly like the idea of extending R6 in a way that mimicks the use of interfaces - which is totally fine ;-)

I just have two questions:

  1. As I wanted to catch up with the lastest state of your package, I wondered what the best practice for keeping "my" version of R6 in sync with your work would be from now on. It's kind of a "reverse pull request", right? But is there any GitHub-based functionality similar to that for pull requests for that which would visualize the diff before merging?

    For now, I did synced your state with mine like this: from within my forked GitHub repo at branch master I ran git pull https://github.com/whc/R6 master, then switched to feat_instances and merged the updated master branch into my branch with git merge master. Is that the way to go or is there something better?

  2. As I pushed my feat_interfaces branch back to my forked repo, I noticed that the pull request was updated as well which in turn triggered a new Travis CI build that failed. Seems like this is due to the fact of knitr being a suggested (as opposed to an imported) package as you now seem to use it as the VignetteBuilder. But how do you manage to tell Travis CI to also install suggested packages so that the test passes? I'm experimenting with using knitr for my vignettes as well. After lots of trial and error today, adding this to .travis.yml seemed to do the trick for me, but I'm not sure if that's the preferred way of handling these cases:

    before_install:
 - Rscript -e 'install.packages("rmarkdown")'

Best, Janko

hadley commented 7 years ago

As per the issue, unfortunately this is out of scope for R6.