Proposal: add a custom signal class for skip(), then sort skip notes by class

[MichaelChirico]

Really liking the recent-ish change to batch skip reasons all in one blob. One further improvement would be better sorting of these skips:

══ Skipped tests (58) ══════════════════════════════════════════════════════════
• fGarch cannot be loaded (1): test-stats.R:26:3
• mapdata cannot be loaded (2): test-maps.R:2:3, test-maps.R:27:3
• MSwM cannot be loaded (1): test-MSwM.R:4:3
• On CRAN (48): test-backcompat.R:2:3, test-base-infer.R:4:3,
  test-base-infer.R:17:3, test-base-infer.R:51:3, test-base.R:4:3,
  test-base.R:12:3, test-base_ts.R:4:3, test-basis.R:6:5,
  test-changepoint.R:4:3, test-changepoint.R:49:3, test-cluster.R:4:3,
  test-cluster.R:27:3, test-cluster.R:81:3, test-cluster.R:127:3,
  test-cluster.R:148:3, test-cluster.R:167:3, test-forecast.R:4:3,
  test-forecast.R:45:3, test-forecast.R:66:3, test-forecast.R:82:3,
  test-forecast.R:98:3, test-forecast.R:117:3, test-forecast.R:133:3,
  test-forecast.R:160:3, test-plotlib.R:4:3, test-plotlib.R:26:3,
  test-plotlib.R:98:3, test-plotlib.R:128:3, test-plotlib.R:150:3,
  test-plotlib.R:215:3, test-plotlib.R:226:3, test-stats-lm.R:7:3,
  test-stats-lm.R:27:3, test-stats-lm.R:40:3, test-stats-lm.R:202:3,
  test-stats-lm.R:363:3, test-stats-lm.R:526:3, test-stats-lm.R:546:3,
  test-stats-lm.R:561:3, test-stats-lm.R:570:3, test-stats.R:222:3,
  test-stats.R:319:3, test-stats.R:420:3, test-stats.R:495:3,
  test-stats.R:522:3, test-stats.R:582:3, test-stats.R:599:3, test-surv.R:238:3
• timeSeries cannot be loaded (6): test-ts.R:4:3, test-ts.R:23:3,
  test-ts.R:44:3, test-ts.R:63:3, test-ts.R:83:3, test-ts.R:128:3

As we see here, the timeSeries skip might be better off grouped with the fGarch, mapdata, and MSwM skips.

One natural way to accomplish this would be for the error signal emitted by skip() to take a custom class, e.g.

c("testthat_error_skip_not_installed", "testthat_error_skip", "error", "condition")
c("testthat_error_skip_on_cran", "testthat_error_skip", "error", "condition")

Then when printing the Skipped tests section, we iterated over skip classes.

Note that approaches building off the text in the skip message will be fraught, e.g. alphabetizing or other pattern matching. The custom signal class approach seems the most generalizable.

[hadley]

How about the ability to add a sort key to the skip object? Then we'd just sort by that, and you'd be free to define as you wish.

[MichaelChirico]

The downside is I'm looking at the output of someone else's suite, so it's not really in my control (unless I send them a PR, nor particularly scalable).

[hadley]

But if it's not in your control, how would you add the custom class?

[MichaelChirico]

The custom class in this case would come from {testthat} (through skip_if_not_installed()), then every package's output sorts " cannot be loaded" correctly.

Of course, a much more light-touch engineering approach is to just reword that particular skip message: "Could not load package ", which I'm fine with. I just filed this in the spirit of custom-signal-condition-all-the-things :)

[hadley]

Yeah, why don't we rework that skip message since we're doing that anyway 😄