openjusticeok / ojoutils

ojoutils
https://openjusticeok.github.io/ojoutils/
GNU General Public License v3.0
0 stars 0 forks source link

Create a description of the change in a value #11

Open brancengregory opened 1 month ago

brancengregory commented 1 month ago

Todo:

For consideration:

anthonyokc commented 1 month ago

Thoughts on binary argument which appends "from {before} to {after} {clean_input_unit}" to the change statement or "at {before} {clean_input_unit}" for the unchanged case?

So "increased by 50 percentage points from 25 to 75 percent" for example. Or "remained unchanged at 25 percent".

brancengregory commented 1 month ago

Yeah, I dig it!

anthonyokc commented 1 month ago

Also argument direction_verbs = c("increased by", "decreased by")? You can swap them out with say direction_verbs = c("rose by", "fell by") if you wanna mix things up and reduce monotony. Simple to implement

brancengregory commented 1 month ago

@anthonyokc, can you explain what the use case for absolute is? I'm now thinking it only matters when include_values = TRUE

anthonyokc commented 1 month ago

include_values isn't commited so I'm not sure which that is. But I think absolute was just to cover the case of when the change was negative so it said "decreased by 32 percent" instead of "decreased by -32 percent" but you're right in that it doesn't have to be an argument, it can just be hardcoded to cover that case. It came from how I was doing it adhoc originally. It's like a vestigial organ lol

brancengregory commented 1 month ago

Gotchu, I think my implementation will cover that. What do you think of this function signature:

describe_change <- function(
  before,
  after,
  input_unit,
  output_unit,
  absolute = TRUE,
  template = "{direction} {change} {unit}",
  phrasing = list(
    increase = "increased by",
    decrease = "decreased by",
    none = "remained unchanged"
  ),
  include_values = FALSE
)

where include_values is the appending

brancengregory commented 1 month ago

I could use change_phrase as the argument rather than phrasing maybe?

anthonyokc commented 1 month ago

I would either use phrases and change direction in template to phrase as well or use direction_phrases just to make the relation between the two explicit.

Also just curious is there a reason to use list instead of named character vector?

brancengregory commented 1 month ago

Also just curious is there a reason to use list instead of named character vector?

Easier to access in the code with $ vs ["item_name"], and also for tibble and purrr friendliness

I actually did it as a named vector first and didn't like it. Thoughts?

brancengregory commented 1 month ago

Actually I'm backtracking that. Named vector makes sense since all the values are strings

brancengregory commented 1 month ago

I think I have enough tests and edge cases covered for an initial version

brancengregory commented 1 month ago

This would be a minor version bump

brancengregory commented 1 month ago
brancengregory commented 1 month ago

@anthonyokc the edge cases keep flowing lol, but hoping to knock these last couple checkboxes and open for review tomorrow

anthonyokc commented 1 month ago

lol this is incredible. This is far beyond what I expected from a 1.0 version of this haha. We should definitely commit to these being the last tasks for now though lol. Cover vast majority of use cases.

brancengregory commented 1 month ago

@anthonyokc I completed all the outstanding tasks and this is ready to go

brancengregory commented 3 weeks ago

@anthonyokc Vignette complete!