-
Notifications
You must be signed in to change notification settings - Fork 14
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
Colour color pt1 #501
Colour color pt1 #501
Conversation
#' @returns named colour argument | ||
#' @keywords internal | ||
#' @noRd | ||
standardise_color_names <- function(...) { |
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.
Do I understand this correctly?
foo <- function(color, ...) {
standardized_color_name(...)
color
}
foo(color = 1)
#> [1] 1
foo(colour = 1)
#> [1] 1
Not sure how much I like messing around with re-assigning values. A simpler implementation could be:
foo <- function(color = colour, colour) {
check_color_colour(color, colour)
color
}
check_color_colour <- function(color, colour) {
if (missing(colour)) {
return(invisible())
}
if (!identical(color, colour)) {
stop("both `color` and `colour` cannot be set")
}
}
# setting only color
foo(1)
#> [1] 1
# setting only colour (turns into color)
foo(colour = 1)
#> [1] 1
# both are identical, that's fine
foo(color = 1, colour = 1)
#> [1] 1
# difference, we have a problem
try(foo(color = 1, colour = 2))
#> Error in check_color_colour(color, colour) :
#> both `color` and `colour` cannot be set
Created on 2022-12-28 with reprex v2.0.2
We'd have to include @params color, colour
but it would keep it explicitly clear to the user.
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.
- Yes, that is all it does.
- I don't like duplicating all arguments. It creates visual confusion, especially with the border stuff and all the different colors. We'll end up with 10 different color arguments and a bunch of comparison functions. Therefore I mimicked the
ggplot2::aes()
approach. The difference to their approach is that they only have a single argument to care about, while we have various. But true, I haven't considered thefoo(color = 1, colour = 2)
approach. Might want to throw a warning if this exists inparent.frame()
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.
Update: 12 different colors ... we have two for the inner grid and checking with exists()
is not enough, because they all exist at this stage. Maybe this will change once you get to #226 *winkwink*.
Therefore I consider keeping it the way it is. After all no harm is done this way. If the user ends up with a unexpected color ... there are more important things to worry about. If their input creates no color at all, fine too. I'd love for R to have a dynamic way to handle this case. Otherwise I simply strive for a consistent user interface.
5cfc5c9
to
fc4c335
Compare
34a22d4
to
e14ece9
Compare
A first part to fix the
color
/colour
mess of ours.color
orcolour
wb_colour()
In a first attempt to align the naming in
openxlsx2
I have added...
argument to (hopefully) all functions usingcolor
/colour
arguments. Every function calling one of these should trigger astandardise_color_names()
orstandardize_colour_names()
function. They convert everycolour
into acolor
argument and vice versa.In a second part we should switch our default to
color
. After all this is used by open xml under the hood. Though that will be a larger and messy search & replace PR.