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

Fixing coord_map() replacement to work with ggplot2 dev version #154

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

mvkorpel
Copy link
Contributor

Related to tidyverse/ggplot2@165e4c9. Should fix issue #144 (which should not occur with ggplot2 2.2.1 from CRAN).

@brendanhoganvt
Copy link

Bravo! This worked for me. I had been torn between my interest in some new development features in ggplot2, and my ggmap needs, so this is a huge help. Thank you!

Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

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

This looks pretty good, and I have found the same problem. Can someone merge it?

@JJ
Copy link

JJ commented Nov 17, 2017

Although I get a different error, which was not present in the development version

Error in if (theme$panel.ontop) { : argumento tiene longitud cero

(meaning 0 length argument)

@scottmmjackson
Copy link
Collaborator

@JJ could you give a little more reproduction information on the error you got?

I pulled down @mvkorpel's branch and merged it with https://github.com/scottmmjackson/ggmap/tree/sj-add-tests locally. The ggmap() test passes but the ggmapplot() test fails with the same error:

Warnings -----------------------------------------------------------------------
1. ggmapplot example works (@test-ggmap.R#13) - ggmapplot syntax deprecated, use ggmap.

2. ggmapplot example works (@test-ggmap.R#13) - fullpage and expand syntaxes deprecated, use extent.

Failed -------------------------------------------------------------------------
1. Error: ggmapplot example works (@test-ggmap.R#13) ---------------------------
object 'fullpage' not found
1: ggmapplot(map) at /Users/scottmmjackson/ggmap/tests/testthat/test-ggmap.R:13
2: ggmap(ggmap, fullpage = fullpage, base_layer = base_layer, maprange = FALSE, expand = FALSE, 
       ggmapplot = TRUE) at /Users/scottmmjackson/ggmap/R/ggmap.R:606
3: eval(args$fullpage) at /Users/scottmmjackson/ggmap/R/ggmap.R:449
4: eval(expr, envir, enclos)

That seems like a separate problem dealing with the use of the eval(names(args)) trick. I think since in this case, it's being used to handle deprecated arguments, maybe we should name those args as part of the signature and wrap their handler in if(!missing(fullpage)), missing() is a very stable function that was around in the S days, so this guarantees broad compatibility.

@scottmmjackson
Copy link
Collaborator

@JJ I think I found what you're talking about. It exists in CRAN checking on master:

Running examples in ‘ggmap-Ex.R’ failed
The error most likely occurred in:

> base::assign(".ptime", proc.time(), pos = "CheckExEnv")
> ### Name: theme_nothing
> ### Title: Make a blank ggplot2 theme.
> ### Aliases: theme_nothing
> 
> ### ** Examples
> 
> 
> 
> # no legend example
> n <- 50
> df <- expand.grid(x = 1:n,y = 1:n)[sample(n^2,.5*n^2),]
> p <- qplot(x, y, data = df, geom = 'tile')
> p
> p + theme_nothing()
Error in if (theme$panel.ontop) { : argument is of length zero
Calls: <Anonymous> ... print.ggplot -> ggplot_gtable -> <Anonymous> -> f -> lapply -> FUN
Execution halted

Probably shouldn't block the merge of this PR.

@JJ
Copy link

JJ commented Dec 10, 2017

@scottmmjackson sorry, it was some time ago and I didn't take a screenshot...

@scottmmjackson
Copy link
Collaborator

@JJ It's all good, I think I fixed it in #186, if you wanted to pull it down and confirm that'd be great!

@scottmmjackson
Copy link
Collaborator

I think this is good to go. The examples are still failing due to theme_nothing but I think that's a separate issue.

@scottmmjackson scottmmjackson merged commit 92deb44 into dkahle:master Feb 9, 2018
@mvkorpel mvkorpel deleted the coord_map branch May 2, 2018 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants