search

bokmann / business_time

Support for doing time math in business hours and days
MIT License
1.27k stars 211 forks source link

Consider dropping external deps: sorted_set and rbtree dependencies? #213

Closed kimyu92 closed 2 years ago

kimyu92 commented 2 years ago

Would it be possible to drop sorted_set and rbtree dependencies for a couple reasons as sorted_set and rbtree?

It required RBTree to perform decently and the external dependency was not suitable for a standard library. The pure ruby fallback implementation was originally meant to be a demonstration of how to write a subclass of Set, and its poor performance was not suitable for use in production.

https://github.com/ruby/set/pull/2#issue-705226116

It also causes issue catastrophic segmentation fault https://github.com/knu/sorted_set/issues/13

rmm5t commented 2 years ago

🤔 Well, the current contract is such that weekdays and holidays are returned as a sorted unique enumerable (currently SortedSet).

Do you have something specific in mind?

kimyu92 commented 2 years ago

sorted unique enumerable

Is array.uniq.sort too slow? Is there a significant gain in real-world performance?

Do you have something specific in mind?

I was thinking simply use array/list if the list wasn't ten of thousands especially for weekdays.

It depends where the code being touched, we can always maintain two data structures, eg. when adding holidays into list, to de-duplicate, we can use set/hash to keep track uniqueness and I think sort in ruby is quick sort, average case should be nlogn which wasn't that bad depending how many times we need to sort it

rmm5t commented 2 years ago

@kimyu92 An array might not be appropriate (we probably should at least use an underlying Set), but still, it might not be quite that simple, because sorting a Set yields an Array.

Note: I think it's totally possible. I'm just trying to balance backwards compatibility. I'd rather this not be a breaking change...but it might have to be to some extent.

I'd be willing to review a PR, but this would likely require custom holidays and weekdays methods (weekdays is already customized).

rmm5t commented 2 years ago

@kimyu92 Please review PR #214. I started thinking about this problem a bit more, and I think the solution is easier that I originally thought.

However, I'm hesitant to immediately merge and release a new version of the business_time gem. I'd like to think on this some more and get more eyes on it. My only concern is if anyone is currently relying on holidays to be returned in sorted order. This might mean that a major version release is warranted warning about the breaking change. Not sure yet.

/cc @bokmann (if you're interested)