Error early if rows could not resolve to anything in data_color()

[olivroy]

Summary

Consider this,

mtcars |>
  gt() |>
  data_color(rows = cyl == 5)

Currently, this errors with a cryptic error:

Error in `get_contrast_ratio()`:
! At least one color defined for each of
  `color_1` and `color_2`.

I have left a review with an alternative approach! Please let me know which one you think is better

Related GitHub Issues and PRs

Discovered while answering #1659

Checklist

• I understand and agree to the Code of Conduct.
• I have listed any major changes in the NEWS.
• I have added testthat unit tests to tests/testthat for any new functionality.

[olivroy]

Alternative PR! (would not be a breaking change, since this currently errors)

Maybe less suprising this way.. I think I prefer the alternative solution, what do you think?

[rich-iannone]

This is a bit tricky API-wise. I like the erroring if you supply a specific row index, row name, or set of them. However, if providing an expression it would be nice to no-op, which is just like the behavior for columns. For rows, I don't know that we currently make the distinction between expression and explicit selection. Also, I don't know if this is the current behavior for other functions with a rows argument (mostly fmt_*() and IIRC some cols_*() fns too).

So, it's worth discussing this a little! I know what I proposed would be a breaking change but I hope it would be a welcome one that fixes some inconsistencies.

[olivroy]

Good idea!

In this case, let's go with the alternative PR (that is the status quo with more informative error)? and I will open a new issue to do that in a folow-up PR.

[rich-iannone]

Disregard the above suggestion, as we're going with a different PR (feel free to close this one).

[olivroy]

Will you open a new PR? The "alternative" PR is this PR in its current state, after I accepted the suggestions #1660 (review).

My suggestion would be to merge this which basically just changes

# before
mtcars |>
  gt() |>
  data_color(rows = cyl == 5)

Error in `get_contrast_ratio()`:
! At least one color defined for each of
  `color_1` and `color_2`.

# this PR
mtcars |>
  gt() |>
  data_color(rows = cyl == 5)

Error in `data_color()`:
!  `rows` resulted in an empty selection.

And address outstanding issue in a follow-up (issue #1665 )?

Does that make sense?

[rich-iannone]

Yes, it makes sense! (Sorry for the confusion here.)

[rich-iannone]

LGTM!