ijlyttle / bsplus

Shiny and R Markdown addons to Bootstrap 3
http://ijlyttle.github.io/bsplus/
Other
146 stars 23 forks source link

bs_accordion doesn't work without attaching the bsplus namespace #34

Closed daattali closed 6 years ago

daattali commented 7 years ago

If I try to create a simple accordion without loading the package (without library(bsplus)), I get an error:

no applicable method for 'bs_attr' applied to an object of class "logical"

Example:

library(shiny)

ui <- fluidPage(
  bsplus::bs_append(bsplus::bs_accordion(id = "foo"), title = "bar", content = "hello world")
)

server <- function(input, output, session) {

}

shinyApp(ui = ui, server = server)
ijlyttle commented 7 years ago

I take advantage of the S3 system - this might be why the namespace is needed.

Does having use library(bsplus) present a problem?

daattali commented 7 years ago

It's not a problem, but I don't think a function should ever REQUIRE its namespace to be explicitly loaded. I should be able to replicate any function calls with ::. If I'm in a package, I much prefer not loading any libraries On Feb 18, 2017 8:45 AM, "Ian Lyttle" notifications@github.com wrote:

I take advantage of the S3 system - this might be why the namespace is needed.

Does having use library(bsplus) present a problem?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ijlyttle/bsplus/issues/34#issuecomment-280846500, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6IFGnP3m1-8CSAlWxLvfelytut1zmBks5rdvYKgaJpZM4MFBwJ .

ijlyttle commented 7 years ago

Let me look at this a bit more on the S3 side to see if I am doing something wrong.

ijlyttle commented 6 years ago

Hey @daattali,

Like so many things in this package, I have let the weeds grow over this issue...

A few days ago, I got a friendly email from CRAN imploring me to add some NAMESPACE directives.

Despite their instructions, I could not for, the life of me, reproduce their error. However, it smelled a lot like this issue you filed MORE THAN A YEAR AGO.

I think I have sorted through the problem now - it seemed to be a weird combination of roxygen and combining S3 and S4 dispatch. I should try to write a blog post about it, but I'm not sure I really understand what went wrong or how I may have fixed it.

In the absence of being able to reproduce their error message, I am hoping that by fixing this issue that you filed, it will satisfy CRAN.

This is a very long way of saying thank you, I will be merging and sending the putative fix to CRAN in the next day or so, and I hope to tend to the rest of these issues before too long.

daattali commented 6 years ago

A friendly email from CRAN is a nice change :p

That is indeed a long way to say thank you -- until I got to the last paragraph I kept thinking "please don't ask me for a favour that involves testing cran things that'll suck up an entire day, please don't...."