The column metric should be untouched

[maurolepore]

As we now copy metric into label, I expected all modifications to
happen to label and none to metric. Instead I see metric is input
as a character but output as a factor:

devtools::load_all()
#> ℹ Loading r2dii.plot
library(testthat)

test_that("leaves `metric` untouched", {
  data <- example_market_share()
  p <- plot_trajectory(data)

  untouched <- sort(unique(data$metric))
  metrics <- sort(unique(p[["layers"]][[2]][["data"]][["metric"]]))
  
  # Passes
  expect_equal(sort(levels(metrics)), untouched)
  
  # Fails
  expect_equal(metrics, untouched)
})
#> Normalizing `production` values to 2020 -- the start year.
#> ── Failure (<text>:15:3): leaves `metric` untouched ────────────────────────────
#> `metrics` (`actual`) not equal to `untouched` (`expected`).
#> 
#> `actual` is an S3 object of class <factor>, an integer vector
#> `expected` is a character vector ('corporate_economy', 'projected', 'target_cps', 'target_sds', 'target_sps')

^{Created on 2021-06-22 by the reprex package (v2.0.0)}

[jdhoffa]

@MonikaFu I'd like your input here, do you also have the expectation that all data wrangling in the plot functions only touch the label column?

I looked into this a bit, and noted that one of the first things that happens to data is that we pass it through prep_trajectory, which immediately renames one of the metrics to target_worse. So we are "touching" the metric column right from the beginning.

I'm happy to look into this and try to only touch the newly created label column, but it is likely to be a bit of a deviation from the current set-up.

[MonikaFu]

I suggest keeping it as it is as working on metric is much easier than working on the label. The metric has a set of values that are always the same, while the label can be anything defined by the user. Touching only the label would require a lot of workarounds in the code which I don't think they are worth it. I can see why modifying metric is not necessarily good practice but I'm tempted to let it slide in this case. Happy to hear your thoughts on it, though.

[jdhoffa]

I would move to close this without action personally.

Although, if we are really worried, we could simply mutate a new "metric" column from the original that we then do the calculations on, so that the original metric column is left "in tact" in the final output plot.

It isn't immediately clear to me how much value that has to the end user.

[MonikaFu]

I'm closing this without action. I don't think it is necessary to act on it at this point.