-
Notifications
You must be signed in to change notification settings - Fork 2.1k
replace parse() with parse_safe() in geom_text() #2867
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
Changes from 14 commits
34c01cc
fe7216a
aa9bfc3
5dd869e
252e7b2
4b6b02f
aa00e13
5a21915
f65ea71
f5a853f
82d6e34
2a089c0
66c9234
8c798ee
2b8b459
637a88e
95025cb
54d87c7
0615062
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -476,9 +476,20 @@ CoordSf <- ggproto("CoordSf", CoordCartesian, | |
if (!is.null(graticule$plot12)) | ||
graticule$degree_label[!graticule$plot12] <- NA | ||
|
||
# parse labels into expressions if required | ||
if (any(grepl("degree", graticule$degree_label))) | ||
graticule$degree_label <- lapply(graticule$degree_label, function(x) parse(text = x)[[1]]) | ||
# Convert the string 'degree' to the degree symbol | ||
parse_ids <- grepl("\\bdegree\\b", graticule$degree_label) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be more informative if called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree! |
||
if (any(parse_ids)) { | ||
graticule$degree_label <- Map( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather not use labels <- as.list(graticule$degree_label)
labels[parse_ids] <- lapply(labels[parse_ids], parse_safe) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is much simpler. For some reason I always forget that we can assign to a subset of elements in a list. However, it still needs the part where only the first element of the expression object is extracted, so we get the element itself not inside an labels <- as.list(graticule$degree_label)
labels[parse_ids] <- lapply(labels[parse_ids], function(x) parse_safe(x)[[1]]) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh but parse_safe is already vectorised so you can skip the lapply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you apply the character vector that hasn’t been coerced to a list There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure. It may be best to work with a full reprex. The following works: parse_safe <- function(text) {
out <- vector("expression", length(text))
for (i in seq_along(text)) {
expr <- parse(text = text[[i]])
out[[i]] <- if (length(expr) == 0) NA else expr[[1]]
}
out
}
graticule <- data.frame(
type = c("E", "E", "E", "E"),
degree_label = c(NA, "abcd", "10 * degree * E", "15 * degree * E"),
stringsAsFactors = FALSE
)
needs_parsing <- grepl("degree", graticule$degree_label)
labels <- as.list(graticule$degree_label)
labels[needs_parsing] <- lapply(labels[needs_parsing], function(x) parse_safe(x)[[1]])
graticule$degree_label <- labels
graticule
#> type degree_label
#> 1 E NA
#> 2 E abcd
#> 3 E 10 * degree * E
#> 4 E 15 * degree * E Do you see a way to make it simpler? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you're correct, it can be made simpler. parse_safe <- function(text) {
out <- vector("expression", length(text))
for (i in seq_along(text)) {
expr <- parse(text = text[[i]])
out[[i]] <- if (length(expr) == 0) NA else expr[[1]]
}
out
}
graticule <- data.frame(
type = c("E", "E", "E", "E"),
degree_label = c(NA, "abcd", "10 * degree * E", "15 * degree * E"),
stringsAsFactors = FALSE
)
needs_parsing <- grepl("degree", graticule$degree_label)
labels <- as.list(graticule$degree_label)
labels[needs_parsing] <- parse_safe(graticule$degree_label[needs_parsing])
graticule$degree_label <- labels
graticule
#> type degree_label
#> 1 E NA
#> 2 E abcd
#> 3 E 10 * degree * E
#> 4 E 15 * degree * E Created on 2018-09-04 by the reprex package (v0.2.0). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that’s exactly what I was imagining. |
||
function(parse_id, label) { | ||
if (parse_id) { | ||
parse(text = label)[[1]] | ||
} else { | ||
as.expression(label)[[1]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we turning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we need the symbol itself, not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One problem with this code chunk as a whole, is that when I read it I can't easily tell what type of object You could fix that with a comment, but it would be even better to use functions where it's obvious what the output type is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we need a symbol, and we have a character vector of length one, then the function to use is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I was confused. This can definitely be simplified. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so my gut feeling was right and the code I suggest below is a bit clearer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, correct. See also my other comment, below. |
||
} | ||
}, | ||
parse_ids, graticule$degree_label | ||
) | ||
} | ||
|
||
graticule | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not valid isn't quite the right turn of phrase - they are valid; they're just empty. We also fixed handling of multiple expressions in a single string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, will try to make the comment more accurate.