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

Basic support to mimick the use of interface classes #81

Closed
wants to merge 4 commits into from

Conversation

jankowtf
Copy link

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
Copy link
Author

jankowtf commented Jul 15, 2016

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

Janko Thyson added 2 commits July 15, 2016 23:14
Added before_install: - Rscript -e 'install.packages("rmarkdown")' in
order to pass Travis CI test
@hadley
Copy link
Member

hadley commented Jul 17, 2017

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

@hadley hadley closed this Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants