-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
ggplot import support #85
Comments
theme elements done in c2b68cb |
Cool. Let's do the rest of these in the update_ggplot_elements branch. |
Actually @dpastoor, instead of putting ggplot2:: everywhere in the code, we might just be able to put a bunch of things like this: geom_histogram <- ggplot2::geom_histogram in a single file that gets loaded before any of these functions are used. Or am I overlooking something? |
As long as it's in a local scope so this doesn't just reintroduce
|
Yeah this would be in a local scope and not carry over into the user's R On Wednesday, August 12, 2015, Bob Carpenter notifications@github.com
|
@dpastoor I'll see if my strategy works and let you know. I think it would be preferable to not have a zillion |
@jgabry the problem I have with that is you now have to track any possible ggplot element you use across the package, so any time you want to add something you have to check to say hmm do all the elements I call here actually exist in, shall we call it, ggplot 'local', so any new function you'd need to check the imports list so to speak and you'll end up with a mess of
On top of that, I don't know if the CRAN people will go for that, I seem to recall reading something about them frowning heavily on that kind of 'ghetto import'. But I absolutely agree that it will also be a bit of mess to find every call and enforce ggplot::, likewise, it will be tedious actually writing the code. What about a sort of middle ground: See if we can define the entry points to where ggplot components may be used in a standalone fashion in another package, maybe that is just Then all we have to do is add the snippet i mentioned in #82 to explicitly load ggplot in those. Inside of shinystan itself that'll always return false since ggplot is attached, so just uses a couple nanoseconds of electrons. |
@dpastoor if we put ggplot2 in Imports and add this
to I think CRAN would care if this were happening in the R/ directory and/or could possibly affect the global environment, but since this is all in inst/ and only relevant while the app is running it would probably be ok. |
@dpastoor The only thing we'd need to worry about for this approach (I think) would be not to give any function we write and use in the app the same name as one in ggplot2. |
@jgabry that sounds like a good approach. I wonder (hope not) if we'd also have to do the same for shiny functions. We could probably mock a little bit by calling shinystan::launch_shinystan (without explicitly loading shinystan) and see what breaks |
I'm a little late on this, but would the import functionality of https://github.com/smbache/import help out? |
I think that import is almost "too elegant" for our purposes. For better or worse this is a hack that gives us the entire flavor of ggplot inside the package, but will also allow single functions to work that depend on ggplot without the explicit attachment that occurs when the shinystan package is loaded (eg the difference between library(shinystan) and shinystan::some_function_depending_on_ggplot. @jgabry can correct me, but I think the |
I've never heard of it but I just read the readme and it looks really On Wednesday, August 19, 2015, Jeffrey Arnold notifications@github.com
|
Thanks for the suggestion. I'll play around with it and see if it can make On Wednesday, August 19, 2015, Jonah Sol Gabry jgabry@gmail.com wrote:
|
@dpastoor yeah hopefully just a temporary fix. I'm not familiar with the On Wednesday, August 19, 2015, Jonah Sol Gabry <jgabry@gmail.com
|
@jgabry so from my reading of it it is an attempt to emulate the python import functionality, therefore this is a 'script level' import. The pro is it makes your code more robust against namespace clashes since you can explicitly control for them through both renaming or selectively importing. The downside, is if it is like python (which it seems to be based on reading) means imports occur at the I think he alludes to this as noted in the package README
I think if we had like a couple ggplot elements that we explicitly wanted to pull in (eg only qplot or the likes) this would be an ideal candidate, however at this point it seems to add an additional dependency and isn't really targeted for, at least at this point, package development |
tracking progress of explicitly calling ggplot
The text was updated successfully, but these errors were encountered: