masking S4 methods

[MikeBadescu]

Referencing #15, I suggest renaming show and removeClass. While the R show issue seems to be fixed in R 3.3.1, there still is a problem if shinyjs is loaded.

In the code below, both show and removeClass fail when shinyjs is loaded but work as expected if we comment out the first line. Loading shinyjs should not interfere with S4 functionality.

library(shinyjs)

setClass("testS4Object",
         representation(
             ID = "numeric",
             Name = "character"
         ),
         prototype(
             ID = NA_real_,
             Name = NA_character_
         )
)

x = new("testS4Object")
show(x)

removeClass("testS4Object")

[daattali]

I 100% agree with you that it's very bad to have functions with the same names as methods functions. But unfortunately shinyjs has a lot of usage already and renaming those functions will cause too much problems for a lot of people, so I'm very hesitant to do that. If I made the package now I would definitely use different names, but I think it might be too late. I hope that makes sense

[MikeBadescu]

You can do the same as you did with colourpicker.

Or, if you do not want to change the current behavior, adding warnings in README and emphasizing the use of shinyjs:: might help.

Indeed, there is a trade-off between how many of the current users use these functions without running into issues vs how many users (current or new; Bioconductor uses S4!) are / will be exposed to the strange side-effect behavior. Since we cannot quantify the trade-off, the decision is subjective.

[daattali]

Even with colour picker I was hesitant and asked on Twitter how I should handle it. But it was a much easier decision because it doesn't have THAT many users, and also it's more used in interactive environments, whereas show/hide are used inside packages that many people use so it'll have a very big impact. show/hide are the "flagship" functions and I'm really afraid to break functionality for a lot of people.

But I do see your point.... if I were to change them, do you have any reasonable suggestions to what they can be changed? show+hide just make the most intuitive sense in my opinion

[MikeBadescu]

To follow your coding style, I suggest showElement and hideElement - the word "element" is used in the documentation and it is used with HTML. There could be a toggleElement if you were to change all of them at the same time.

Playing with fire, there is display - not defined in base R packages, but in xtable, which is used to create nice reports from shiny (guilty) - I would not recommend this one.

For removeClass, we could have removeCssClass, removeHtmlClass, removeCSSClass, removeHTMLClass, removeElementClass, removeClassCSS, etc. Of course, the same can be applied to addClass and toggleClass.

[daattali]

Ok, thanks for your suggestions. I can promise you that in the coming weeks I won't be changing anything because I have too many other things going on, but I do promise I'll give it some thought..... :) Not the response you were looking for, but hopefully better than nothing! I'll ask more people and get more opinions

[MikeBadescu]

Thank you!

[daattali]

@MikeBadescu I've added synonyms for all the show/hide/addClass/etc functions, and mention them at the top of the corresponding function documentation. I really don't think I'll ever remove existing functions, especially the hide/show since they're the flagship functions and are used in so much code. I hope this compromise is sufficient

[MikeBadescu]

Thank you!

[MikeBadescu]

Since you got me thinking about your solution for the default values (#80), would it make sense to add warning messages for shinysj functions that have the same name as S4? As in, when shinyjs::show() is called, it would display a warning, something like "Just letting you know that you called the shinyjs function, not the S4 one; if you do not want to see this message, use showClass()". And after few years, you could deprecate show().

[daattali]

Since you bring it up again, I do think I should add a note to the documentation that mentions this. I'm a little hesitant to show a message in the console because it's such a widely used function and an extremely small fraction of people run into this masking issue. As you can guess, I'm even less comfortable with the idea of deprecating the show/hide functions at any point in the future, because it will be chaos for a lot of apps.

Do you think it's ok if I just make a note in the function doc that says "if you use S4 classes or see weird behaviour when using 'show()' then you should try using 'showElement()' instead"?

[MikeBadescu]

Sur,e that would help and it is the place where people would look.