Address review comments

NEWS.md

@@ -5,7 +5,7 @@ R6 2.5.1.9000
 
 * `R6Class()` now prints a message when a `finalize` method is public instead of private.
 
-* When subclass' `cloneable` property differs from superclass', the former will override the latter (@IndrajeetPatil, #247).
+* When subclass allows cloning, while superclass has turned cloning off, superclass will override subclass' cloning properties. (@IndrajeetPatil, #247).
 
 R6 2.5.1
 ========

R/new.R

@@ -13,13 +13,22 @@ generator_funs$new <- function(...) {
     if (!identical(portable, inherit$portable))
       stop("Sub and superclass must both be portable or non-portable.")
 
+    # If `cloneable` property differs between sub and superclass
+    # - super will override sub if super doesn't allow cloning
+    # - sub will override super if super allows cloning
     if (!identical(cloneable, inherit$cloneable)) {
-      message(c(
-        "Sub and superclass have different cloneable properties. ",
-        "Subclass property will override superclass property."
-      ))
+      if (inherit$cloneable) {
+        inherit[["public_methods"]][["clone"]] <- NULL
+      }
+
+      if (!inherit$cloneable) {
+        message(c(
+          "Subclass wants to allow cloning, but superclass has turned it off. ",
+          "Therefore, cloning will also be turned off for subclass."
+        ))
 
-      inherit[["public_methods"]][["clone"]] <- NULL
+        public_methods[["clone"]] <- NULL
+      }
     }
 
     # Merge fields over superclass fields, recursively --------------

tests/testthat/test-cloning-inheritance.R

@@ -1,6 +1,4 @@
 test_that("Subclass can override superclass' cloneable property", {
-  msg <- "Sub and superclass have different cloneable properties."
-
   # superclass cloneable ---------------------
 
   Creature <- R6Class("Creature", cloneable = TRUE)
@@ -10,16 +8,16 @@ test_that("Subclass can override superclass' cloneable property", {
   expect_s3_class(sheep$clone(), "Sheep")
 
   Human <- R6Class("Human", inherit = Creature, cloneable = FALSE)
-  expect_message(human <- Human$new(), msg)
+  expect_message(human <- Human$new(), NA)
   expect_error(human$clone(), "attempt to apply non-function")
 
   # superclass non-cloneable  ---------------------
 
   Creature <- R6Class("Creature", cloneable = FALSE)
 
   Sheep <- R6Class("Sheep", inherit = Creature, cloneable = TRUE)
-  expect_message(sheep <- Sheep$new(), msg)
-  expect_s3_class(sheep$clone(), "Sheep")
+  expect_message(sheep <- Sheep$new(), "Subclass wants to allow cloning, but superclass has turned it off.")
+  expect_error(sheep$clone(), "attempt to apply non-function")
 
   Human <- R6Class("Human", inherit = Creature, cloneable = FALSE)
   expect_message(human <- Human$new(), NA)