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

Colour color pt1 #501

Merged
merged 5 commits into from
Dec 31, 2022
Merged

Colour color pt1 #501

merged 5 commits into from
Dec 31, 2022

Conversation

JanMarvin
Copy link
Owner

@JanMarvin JanMarvin commented Dec 28, 2022

A first part to fix the color/colour mess of ours.

  • We currently have functions using arguments color or colour
  • We have wb_colour()
  • R uses both, ggplot and many others use both too

In a first attempt to align the naming in openxlsx2 I have added ... argument to (hopefully) all functions using color/colour arguments. Every function calling one of these should trigger a standardise_color_names() or standardize_colour_names() function. They convert every colour into a color 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.

  wb_color <- wb_workbook() %>%
    wb_add_worksheet(tabColor = "green") %>%
    wb_add_fill(color = wb_color("blue")) %>%
    wb_add_border(
      dims   = "G12:H13",
      left_color   = wb_color("red"),
      right_color  = wb_color("blue"),
      top_color    = wb_color("green"),
      bottom_color = wb_color("yellow")
    )

  wb_colour <- wb_workbook() %>%
    wb_add_worksheet(tabColour = "green") %>%
    wb_add_fill(colour = wb_colour("blue")) %>%
    wb_add_border(
      dims   = "G12:H13",
      left_colour   = wb_colour("red"),
      right_colour  = wb_colour("blue"),
      top_colour    = wb_colour("green"),
      bottom_colour = wb_colour("yellow")
    )

#' @returns named colour argument
#' @keywords internal
#' @noRd
standardise_color_names <- function(...) {
Copy link
Collaborator

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

  1. Yes, that is all it does.
  2. 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 the foo(color = 1, colour = 2) approach. Might want to throw a warning if this exists in parent.frame()

Copy link
Owner Author

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.

@JanMarvin
Copy link
Owner Author

Tomorrow I intend to merge #501 and #502 with main. Could be a nightmare to merge with #471, but there are still a couple of winter evenings left

@JanMarvin JanMarvin merged commit cf3f888 into main Dec 31, 2022
@JanMarvin JanMarvin deleted the colour_color branch December 31, 2022 01:12
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.

None yet

2 participants