mhahsler / arules

Mining Association Rules and Frequent Itemsets with R
http://mhahsler.github.io/arules
GNU General Public License v3.0
194 stars 42 forks source link

`show` should be used instead of `print` for S4 objects #50

Closed daattali closed 5 years ago

daattali commented 5 years ago

When printing an S4 object to the console, such as APparameter, the show() function should be used rather than print().

This line (and possibly others) violate this, which can cause problems. Read the "Show method" section here for more details.

mhahsler commented 5 years ago

I think the issue is something else.

From ? show

When the methods package is attached, print will call show for R objects with formal classes if called with no optional arguments.

Show does not work either after loading shinyjs

> library(arules)
> library(shinyjs)
> test <- as(list(supp = 0.01, conf = 0.8), "APparameter")
> test
 confidence minval smax arem  aval originalSupport maxtime support minlen maxlen target   ext
        0.8    0.1    1 none FALSE            TRUE       5    0.01      1     10  rules FALSE
> print(test)
Error: shinyjs: could not find the Shiny session object. This usually happens when a shinyjs function is called from a context that wasn't set up by a Shiny session.
> show(test)
Error: shinyjs: could not find the Shiny session object. This usually happens when a shinyjs function is called from a context that wasn't set up by a Shiny session.
daattali commented 5 years ago

Thanks for the quick response.

Since this would be inside a package, CRAN would not accept it when show is not namespaced (this requirement only happened in the last couple of years, exactly for this reason). Inside your actual code, it should be methods::show(test)

mhahsler commented 5 years ago

I am still confused. shinyjs on purpose masks show(). I guess it should warn that it cannot find the session and then redirect the show to the original show in methods.

> print(test)
Error: shinyjs: could not find the Shiny session object. This usually happens when a shinyjs function is called from a context that wasn't set up by a Shiny session.
> traceback()
8: stop(sprintf("shinyjs: %s", x), call. = FALSE)
7: errMsg(paste("could not find the Shiny session object. This usually happens when a", 
       "shinyjs function is called from a context that wasn't set up by a Shiny session."))
6: getSession()
5: jsFuncHelper(fxn, params)
4: function (id = NULL, anim = FALSE, animType = "slide", time = 0.5, 
       selector = NULL) 
   {
       fxn <- "show"
       params <- list(id = id, anim = anim, animType = animType, 
           time = time, selector = selector)
       jsFuncHelper(fxn, params)
   }(new("APparameter", confidence = 0.8, minval = 0.1, smax = 1, 
       arem = "none", aval = FALSE, originalSupport = TRUE, maxtime = 5, 
       support = 0.01, minlen = 1L, maxlen = 10L, target = "rules", 
       ext = FALSE))
3: print.default(test)
2: print(test)
1: print(test)

In the meanwhile, the output can be suppress by calling apriori with control = list(verbose = FALSE)

library("shinyjs")
library("arules")
data("Adult")
## Mine association rules.
rules <- apriori(Adult, 
    parameter = list(supp = 0.5, conf = 0.9, target = "rules"), control = list(verbose = FALSE))
summary(rules)
daattali commented 5 years ago

While it was not a particularly good idea to have a function named show() (masking any function is not a good idea, but as long as it's not a base function it's ok), that alone should not be responsible for S4 objects not printing. This is the exact reason why CRAN has the requirement that any function call outside of the base package must be namespaced, because there were other instances where calling show() inside a package, or calling other functions, would call the wrong function. By namespacing internal code in a package, that is avoided. The current print() in the code does not have to be namespaced because it's from the base package, but print() is not the correct function to call on an S4 object.

shinyjs can write hacky code that would try to forward requests to methods::show() but that is not a valid solution.

Any code that wishes to show an S4 to console should use methods::show() instead of print(), that is the correct thing to do, and it would solve the problem here.

mhahsler commented 5 years ago

I agree with that it is better to directly call show and I have uploaded a fix to github. It will take a while till it will show up in the release code.

However, you need to talk with R-devel about using show. I think there is a bug in print.default which dispatches for S4 objects to show in the global namespace, but should dispatch to methods::show. This is why your show function causes problems.

daattali commented 5 years ago

I did in fact speak with R devel about this issue a few years ago, and it was fixed: https://github.com/wch/r-source/commit/38ea40dcd0353af16d35296ee621338c49ae48c9

That issue really was a bug because simply typing an object at the console would not dispatch correctly.

However, the issue you're referring to is that the print method doesn't dispatch to show. I don't think that's considered a bug, because print should not be called on S4 objects. Although I do agree that this needs to be documented more clearly. Whether or not print should call show is a philosophical/design question which I'm unqualified to comment on :)

mhahsler commented 5 years ago

I think the person that fixed the dispatch for typing the object into the console should have also fixed print.

The manual page for print.default says:

When the ‘methods’ package is attached, ‘print’ will call ‘show’ for R objects with formal classes if called with no optional arguments.

If you don't want to get people complaining again, then you should probably ask them to also fix print.default. Good luck :)

daattali commented 5 years ago

Good find! I was going to say that what the documentation says is consistent with the behaviour, because methods was not attached. But I just tried it out and it looks like even if you do attach methods, it doesn't call the correct show().

Case in point: (I will report this, thanks!)

library(methods)
show <- function(...) message("hijacked!")
setClass("Test", representation(test = "character"))
test <- new("Test")
print(test)