Implemented S7 by swapping the rate class and subclasses from S3
#1154
Conversation
Need to update tests, documentation. Note that order of arguments are different for subclasses in the current structure--change that.
1. Very ugly way to override super's max_times in rate_delay 2. Changed test cases a bit to correspond with new S7 syntax, but getting random errors so need to look into more
Couple caveats/notes: Needed to change a few tests to match new S7 syntax. Current method of overriding super's default values is inelegant (see `rate_delay`), but functional. Would like to change. Still needs some documentation.
Changed class construction to conform with new API. Removed default values in properties as they are listed in constructors.
| "rate", | ||
| package = "purrr", | ||
| properties = list( | ||
| jitter = new_property(class_logical, validator = function(value) { |
There was a problem hiding this comment.
These really illustrate the need for scalar properties, like we have in ellmer. Definitely something we should try to add to S7.
| state = new_property(class_environment) | ||
| ), | ||
| constructor = function(jitter = TRUE, max_times = 3, state = env(i = 0L)) { | ||
| force(jitter) |
There was a problem hiding this comment.
Why are you forcing in constructors?
| rate <- new_class( | ||
| "rate", | ||
| package = "purrr", | ||
| properties = list( |
There was a problem hiding this comment.
I wonder if the problem you're facing with max_time is actually just an illustration that the whole design here is wrong — why does the base case have jitter and max_times? I think it might be better to redesign this class hierarchy so that rate doesn't have any properties, but instead has a set of generics like rate_next(), rate_is_exhausted() etc? Then rate_sleep() becomes a regular function and everything else simplifies. Is this something you'd be interested in working on? If so, we can spend a bit more time sketching out the design and then you could take another pass.
There was a problem hiding this comment.
Ahh that does sound a lot cleaner. I'd be more than happy to help!
There was a problem hiding this comment.
Awesome, thanks! I think the place to start would be to sketch out a new rate_sleep() along with a few lower level generics. Maybe something like this?
rate_sleep <- function(rate, quiet = TRUE) {
stopifnot(is_rate(rate))
if (rate_expired(rate)) {
cli::cli_abort(
c(
"This `rate` object has already been called the maximum number of times.",
i = "Do you need to reset it with `rate_reset()`?"
)
)
}
delay <- rate_get_delay(rate)
if (quiet) {
signal(msg, class = "purrr_message_rate_retry", delay = delay)
} else {
cli::cli_inform(sprintf("Retrying in {length} second{?s}."))
}
Sys.sleep(delay)
}
|
Something like this? There's still some cruft from the original structure, but is the main logic ok? |
Fixes #1129
Notes/caveats:
I changed some tests to account for the new syntax of S7 objects--eg. changing
$to@. Additionally, the new constructor forrateobjects is called justrate()instead ofnew_rate()so I also changed some variable names in the tests to not conflict with that.I don't like line 71 in rate.R. There, I have a subclass with a different default value of a property than the parent class. I want to override the super's default value for
max_times, but when I just includemax_timesas a property of the subclass with a different default, the constructed object's value goes to that of the parent. (3 instead of Infinity forrate_delay).I got around it by checking if
max_timeswas supplied as an argument and providing the correct default value if it wasn't.I didn't write change any documentation, so if that needs to be changed I will fix that. The changes are mainly internal, but it may be good to document what certain things are doing so that if S7 changes the purpose of the code can still be understood.
Mentioning @kbodwin since this PR is part of the awesome Independent Study: Advanced R with Dr Bodwin this quarter!