Skip to content
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

Allow addition of custom objects #2309

Merged
merged 9 commits into from Oct 30, 2017
Merged

Conversation

thomasp85
Copy link
Member

As discussed. This PR adds the possibility of developers writing custom behaviour for adding other objects than those defined by ggplot2 to a gg object.

It works by falling back to the add_to_ggplot() generic in add_ggplot(). A potential change to the PR would be to write add_to_ggplot() methods for all the build in classes as well thus completely removing the if else check from add_ggplot()

@thomasp85 thomasp85 requested a review from hadley October 24, 2017 18:49
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should eliminate add_ggplot() by providing the appropriate methods to add_to_ggplot().

(It'll probably also need a different name - maybe ggplot_add()).

@@ -60,46 +60,88 @@ add_ggplot <- function(p, object, objectname) {
if (is.null(object)) return(p)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now be a method of the generic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh - Didn’t knew you could dispatch on NULL

#' a ggplot with [+.gg].
#'
#' @param object An object to add to the plot
#' @param p The ggplot object to add `object` to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think plot and object_name would be better argument names

ggplot_add.test_object <- function(object, p, objectname) {
10
}
test_that("Methods can be defined for adding custom objects", {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can define the method inside the test, just to make things slightly cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I’m having problems getting dispatch to work correctly in the test. Moving it out fixed it locally but it still throws an error on travis..?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any idea why methods defined inside test_that expressions are not being dispatched to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's something to do with the weird way in which S3 dispatch happens. I forget all the details, but just move it outside for now.

@hadley
Copy link
Member

hadley commented Oct 30, 2017

Does R CMD check pass locally?

@thomasp85
Copy link
Member Author

I have some strange (libc++abi.dylib: __cxa_guard_acquire detected deadlock) errors in unrelated tests that I'm currently troubleshooting

@thomasp85
Copy link
Member Author

OK - I'm up to speed again.

No It doesn't pass locally. The test method won't get dispatched to when called as part of Check, but works fine when pasted into the console. I guess the problem is the same - that S3 methods are not properly registered when defined within tests...

No idea how to properly test this then...

@hadley
Copy link
Member

hadley commented Oct 30, 2017

I think you can just drop the test - if the S3 dispatch was failing, every plot would break.

@thomasp85
Copy link
Member Author

I never thought I should live to hear you ask me to remove a test - this will haunt you 😛

# Conflicts:
#	NEWS.md

Merge branch 'tidyverse/master' into custom-object-add

# Conflicts:
#	NEWS.md
@hadley hadley merged commit 3c16f9c into tidyverse:master Oct 30, 2017
@hadley
Copy link
Member

hadley commented Oct 30, 2017

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants