Optional contexts feature #39
Conversation
Merge master
* Add environments option to retriable * Make it reconfigurable * README * Move environment validation to Environment class * Don't use method_missing in Environment; add reset! method * README * No shenanigans
…hods with environments (#3) * Add validation for the :on param * Disallow underloading any methods * Rename validate\! * README
Merge master
|
p.s. @kamui prolly a good case for "squash + merge" instead of regular merge |
|
any thoughts, @kamui? |
|
ping @kamui |
|
ping |
|
@kamui, are you still maintaining this or should we just fork the project? |
|
I apologize for the long wait, I just haven't had a lot of free time. I did look at this PR a few times and had a pending review. There were a few implementation choices that I had some issues with, but at the time I didn't have a better solution, so I put it off to think some more on it, I ended up spiking on some experimenting with the API and implementation myself to see if I could figure out a simpler implementation that didn't require patching config or subclassing Hash and I came up with: BTW, I like how you reformatted the README, I'd like to borrow that if you don't mind. |
|
sure do whatever you want with the readme - |
| @@ -6,15 +6,20 @@ | |||
| module Retriable | |||
| module_function | |||
|
|
|||
| def self.configure | |||
| def configure | |||
There was a problem hiding this comment.
@kamui i don't know if you want to grab this change.. the self is gratuitous because of the module_function
| @@ -40,7 +45,7 @@ def retriable(opts = {}) | |||
| base_interval: base_interval, | |||
| multiplier: multiplier, | |||
| max_interval: max_interval, | |||
| rand_factor: rand_factor, | |||
| rand_factor: rand_factor | |||
There was a problem hiding this comment.
this was also just some housekeeping
| end | ||
|
|
||
| return intervals if rand_factor.zero? |
There was a problem hiding this comment.
the changes in this file are also just housekeeping changes - this line is unnecessary (i mean, it saves a single ruby call... but we are usually retrying operations that take at least seconds)
There was a problem hiding this comment.
I'm going to keep this line. The reason I like it is because if rand_factor is 0, basically, don't randomize. This line basically does that from a logical step point of view: if it's 0, don't call randomize. It skips some math, map allocates memory for a new Array. I know it's super minimal, but this logically makes sense to me.
There was a problem hiding this comment.
you already have that line...
There was a problem hiding this comment.
(i get what it does; feel free to keep it NBD i just find that less code = more readable code)
There was a problem hiding this comment.
Yeah, it makes sense in randomize, since it should exit early if rand_factor is 0, but I also think it makes sense in intervals too. If it can skip the map, randomize method invocation, allocating a new array, why not have it be more efficient?
|
If you have the time and are inclined, a PR of the README reformatting would be awesome. |
Replaces #33
Before we settled on using
Retriable, we had our own gem for retries called Pester. We went withRetriablein the end but there was one killer feature ofPesterthat we have long wanted inRetriableso we added it.The documentation should explain things but let me know if anything is unclear
@kamui / @slpsys