Comment 11 for bug 1635737

Revision history for this message
Dan Wells (dbw2) wrote :

Sorry for the delay.

So, here is some working code along the lines I mentioned. Since interval_to_seconds() lives in OpenSRF, the meat of this version is actually an OpenSRF branch:

http://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/dbwells/lp1635737_interval_to_seconds_contextual

With that in place, the EG changes are of course pretty simple:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1635737_better_interval_to_seconds

There may be more places to use this new skill, but these are the ones highlighted in the discussion so far.

This works pretty well for me, but I do wonder a few things:

1) The new behavior is completely optional. If we feel it is *generally* more correct behavior, we could consider reversing that, and making it the default with a context of "now()", with a compatibility switch for "simple"-version calculations. That would obviously be more disruptive short-term, but if it pays off long-term, maybe worth it.

2) It would be pretty simple to sub out some of the logic to a new "interval_to_duration" function in OpenSRF::Utils. This current branch is great for simplicity of the upper layer changes (and we could keep that as an option), but forcing it all down to seconds is probably some unnecessary work in many cases. We could instead use this new duration directly for the math, and everything would work fine. I think that would actually be truest to the spirit of the original discussion/branch, but the round-tripping of the interval tripped (*ahem*) things up a bit.

I'd like to go ahead and try extracting the "interval_to_duration()" sub-layer if we go this direction, but also wanted to get some "early" feedback on this overall. Thanks!