Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow use with simple custom objects #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

amarzot-yesware
Copy link

This PR is includes the work needed to be able to use the interval tree with simple classes and structs, such as this:

CS = Struct.new(:begin, :end, :value) # CS = CustomStruct
itv = [
  CS.new(0,3, "Value"), CS.new(0,3, "Value"),
  CS.new(1,4, %w[This is an array]), CS.new(3,5, %w[A second an array])
]
t = IntervalTree::Tree.new(itv)
pp t.search(2)     
#=> [#<struct CS begin=0, end=3, value="Value">,
#    #<struct CS begin=1, end=4, value=["This", "is", "an", "array"]>]

I made the following changes:

  1. Use begin instead of first, and end instead of last

  2. Change point_search such that it does not assume the interval
    has the include? method. This allows for custom objects to
    be used as intervals.

  3. Add specs for the behavior of the tree with custom
    interval objects. Specs include:

    • Tree creation
    • Searching for custom objects
    • Searching with custom objects
  4. Update the readme to include examples with custom objects, and
    update the notes to reflect how custom objects behave.

Change point_search such that it does not assume the interval
has the `include?` method. This allows for custom objects to
be used as intervals.

Add specs for the behavior of the tree with custom
interval objects. Specs include:
  * Tree creation
  * Searching for custom objects
  * Searching with custom objects

Update the readme to include examples with custom objects, and
update the notes to reflect how custom objects behave.
@danielpetri1
Copy link

Great addition. This is exactly what this gem was missing. Thank you! Hope it gets accepted soon.

@amarzot-yesware
Copy link
Author

amarzot-yesware commented Jun 29, 2021

@danielpetri1 If you want to use objects with data right now, you can use this pattern. The include? definition is for point search to work properly.

  CustomStruct = Struct.new(:begin, :end, :value) do 
    def include?(other)
      self.begin <= other && other < self.end
    end
    def first
      self.begin
    end
    def last
      self.end
    end
  end

Copy link
Contributor

@ZimbiX ZimbiX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a good idea.

I'm not sure if the switch away from #include? has an impact on ranges which include vs exclude the ending. Given all the specs pass, perhaps we were missing a spec for that? Or maybe everything always just gets converted to one form, and we lose that information. I can't remember =P I'll need to look into it when I have more time.

Cheers for the contribution!

@amarzot-yesware
Copy link
Author

amarzot-yesware commented Jul 13, 2021

@ZimbiX

I'm not sure if the switch away from #include? has an impact on ranges which include vs exclude the ending.
... Or maybe everything always just gets converted to one form, and we lose that information.

That's exactly what happens. There is a line in the README which says:

Two-dotted full-closed intervals (first..last) are also accepted and internally converted to half-closed intervals.

I have added a note in this PR explaining how this idea is extended to non-range objects

When using custom objects the tree assumes the "left-closed and right-open" style (i.e. the end property is not
considered to be inside the interval).

@amarzot-yesware
Copy link
Author

You know, after thinking this over for a few days, I think that it actually makes more sense, in the long run, to lean into the range interface. I'm thinking about the annoying ensure_exclusive_end function, and that we could get rid of it entirely if we relied on include?. I feel like it makes more sense for the objects themselves to decide what they include, as opposed to the tree making that decision. Also trying to keep the methods we use from range minimal and documented will allow people to just implement the interface instead of inheriting from the object. I think this would be a breaking change though, so for now I still like this PR, and maybe we can work towards a major version release with lots of nice functionality?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants