Merge pull request #1818 from pry/ring-refactoring

ring: rewrite the class to improve API

lib/pry.rb

@@ -64,11 +64,11 @@ class Pry
   # The default prompt; includes the target and nesting level
   DEFAULT_PROMPT = [
                     proc { |target_self, nest_level, pry|
-                      "[#{pry.input_ring.size}] #{pry.config.prompt_name}(#{Pry.view_clip(target_self)})#{":#{nest_level}" unless nest_level.zero?}> "
+                      "[#{pry.input_ring.count}] #{pry.config.prompt_name}(#{Pry.view_clip(target_self)})#{":#{nest_level}" unless nest_level.zero?}> "
                     },
 
                     proc { |target_self, nest_level, pry|
-                      "[#{pry.input_ring.size}] #{pry.config.prompt_name}(#{Pry.view_clip(target_self)})#{":#{nest_level}" unless nest_level.zero?}* "
+                      "[#{pry.input_ring.count}] #{pry.config.prompt_name}(#{Pry.view_clip(target_self)})#{":#{nest_level}" unless nest_level.zero?}* "
                     }
                    ]
 

lib/pry/commands/cat/input_expression_formatter.rb

@@ -36,7 +36,7 @@ class Pry
       end
 
       def normalized_expression_range
-        absolute_index_range(opts[:i], input_expressions.length)
+        absolute_index_range(opts[:i], input_expressions.count)
       end
     end
   end

lib/pry/commands/edit.rb

@@ -186,7 +186,7 @@ class Pry
       when eval_string.strip != ""
         eval_string
       else
-        _pry_.input_ring.reverse_each.find { |x| x && x.strip != "" } || ""
+        _pry_.input_ring.to_a.reverse_each.find { |x| x && x.strip != "" } || ""
       end
     end
 

lib/pry/ring.rb

@@ -1,118 +1,83 @@
 class Pry
-  # A ring is an array to which you can only add elements. Older entries are
-  # removed progressively, so that the array never contains more than N
-  # elements.
-  #
-  # Rings are used by Pry to store the output of the last commands.
+  # A ring is a thread-safe fixed-capacity array to which you can only add
+  # elements. Older entries are overwritten as you add new elements, so that the
+  # ring can never contain more than `max_size` elemens.
   #
   # @example
-  #   ring = Pry::Ring.new(10)
+  #   ring = Pry::Ring.new(3)
   #   ring << 1 << 2 << 3
-  #   ring[0] # => 1
-  #   ring[1] # => 2
-  #   10.times { |n| ring << n }
-  #   ring[0] # => nil
-  #   ring[-1] # => 9
+  #   ring.to_a #=> [1, 2, 3]
+  #   ring << 4
+  #   ring.to_a #=> [4, 2, 3]
+  #
+  #   ring[0] #=> 2
+  #   ring[-1] #=> 4
+  #   ring.clear
+  #   ring[0] #=> nil
+  #
+  # @api public
+  # @since v0.12.0
   class Ring
-    include Enumerable
-
-    # @param [Integer] size Maximum amount of objects in the array
-    def initialize(size)
-      @max_size = size
-
-      @hash  = {}
-      @count = 0
-    end
-
-    # Pushes an object at the end of the array
-    # @param [Object] value Object to be added
-    def <<(value)
-      @hash[@count] = value
-
-      if @hash.size > max_size
-        @hash.delete(@count - max_size)
-      end
-
-      @count += 1
-
-      self
-    end
-
-    # @overload [](index)
-    #   @param [Integer] index Index of the item to access.
-    #   @return [Object, nil] Item at that index or nil if it has been removed.
-    # @overload [](index, size)
-    #   @param [Integer] index Index of the first item to access.
-    #   @param [Integer] size Amount of items to access
-    #   @return [Array, nil] The selected items. Nil if index is greater than
-    #     the size of the array.
-    # @overload [](range)
-    #   @param [Range<Integer>] range Range of indices to access.
-    #   @return [Array, nil] The selected items. Nil if index is greater than
-    #     the size of the array.
-    def [](index_or_range, size = nil)
-      unless index_or_range.is_a?(Integer)
-        range = convert_range(index_or_range)
-        return range.begin > @count ? nil : range.map { |n| @hash[n] }
-      end
-
-      index = convert_index(index_or_range)
-      return @hash[index] unless size
-      return if index > @count
-
-      end_index = index + size
-      (index...[end_index, @count].min).map { |n| @hash[n] }
-    end
-
-    # @return [Integer] Amount of objects in the array
-    def size
-      @count
-    end
-    alias count size
-    alias length size
-
-    def empty?
-      size == 0
-    end
-
-    def each
-      ((@count - size)...@count).each do |n|
-        yield @hash[n]
-      end
-    end
-
-    def to_a
-      ((@count - size)...@count).map { |n| @hash[n] }
-    end
-
-    # @return [Hash] copy of the internal @hash history
-    def to_h
-      @hash.dup
-    end
-
-    def pop!
-      @hash.delete @count - 1
-      @count -= 1
-    end
-
-    def inspect
-      "#<#{self.class} size=#{size} first=#{@count - size} max_size=#{max_size}>"
-    end
-
-    # @return [Integer] Maximum amount of objects in the array
+    # @return [Integer] maximum buffer size
     attr_reader :max_size
 
-    private
+    # @return [Integer] how many objects were added during the lifetime of the
+    #   ring
+    attr_reader :count
 
-    def convert_index(n)
-      n >= 0 ? n : @count + n
+    # @param [Integer] max_size Maximum buffer size. The buffer will start
+    #   overwriting elements once its reaches its maximum capacity
+    def initialize(max_size)
+      @max_size = max_size
+      @mutex = Mutex.new
+      clear
     end
 
-    def convert_range(range)
-      end_index = convert_index(range.end)
-      end_index += 1 unless range.exclude_end?
+    # Push `value` to the current index.
+    #
+    # @param [Object] value
+    # @return [self]
+    def <<(value)
+      @mutex.synchronize do
+        @buffer[count % max_size] = value
+        @count += 1
+        self
+      end
+    end
 
-      Range.new(convert_index(range.begin), [end_index, @count].min, true)
+    # Read the value stored at `index`.
+    #
+    # @param [Integer, Range] index The element (if Integer) or elements
+    #   (if Range) associated with `index`
+    # @return [Object, Array<Object>, nil] element(s) at `index`, `nil` if none
+    #   exist
+    def [](index)
+      @mutex.synchronize do
+        return @buffer[(count + index) % max_size] if index.is_a?(Integer)
+        return @buffer[index] if count <= max_size
+
+        # Swap parts of array when the array turns page and starts overwriting
+        # from the beginning, then apply the range.
+        last_part = @buffer.slice([index.end, max_size - 1].min, count % max_size)
+        (last_part + (@buffer - last_part))[index]
+      end
+    end
+
+    # @return [Array<Object>] the buffer as unwinded array
+    def to_a
+      return @buffer.dup if count <= max_size
+
+      last_part = @buffer.slice(count % max_size, @buffer.size)
+      last_part + (@buffer - last_part)
+    end
+
+    # Clear the buffer and reset count.
+    # @return [void]
+    def clear
+      @mutex.synchronize do
+        @buffer = []
+        @count = 0
+      end
     end
   end
 end

spec/ring_spec.rb

@@ -1,71 +1,107 @@
 require_relative 'helper'
 
 describe Pry::Ring do
-  before do
-    @ring = Pry::Ring.new(10)
-    @populated = @ring.dup << 1 << 2 << 3 << 4
+  let(:ring) { described_class.new(3) }
+
+  describe "#<<" do
+    it "adds elements as is when the ring is not full" do
+      ring << 1 << 2 << 3
+      expect(ring.to_a).to eq([1, 2, 3])
+    end
+
+    it "overwrites elements when the ring is full" do
+      ring << 1 << 2 << 3 << 4 << 5
+      expect(ring.to_a).to eq([3, 4, 5])
+    end
   end
 
-  it 'should have a maximum size specifed at creation time' do
-    expect(@ring.max_size).to eq 10
+  describe "#[]" do
+    context "when the ring is empty" do
+      it "returns nil" do
+        expect(ring[0]).to be_nil
+      end
+    end
+
+    context "when the ring is not full" do
+      before { ring << 1 << 2 << 3 }
+
+      it "reads elements" do
+        expect(ring[0]).to eq(1)
+        expect(ring[1]).to eq(2)
+        expect(ring[2]).to eq(3)
+
+        expect(ring[-1]).to eq(3)
+        expect(ring[-2]).to eq(2)
+        expect(ring[-3]).to eq(1)
+      end
+
+      it "reads elements via range" do
+        expect(ring[1..2]).to eq([2, 3])
+        expect(ring[-2..-1]).to eq([2, 3])
+      end
+    end
+
+    context "when the ring is full" do
+      before { ring << 1 << 2 << 3 << 4 << 5 }
+
+      it "reads elements" do
+        expect(ring[0]).to eq(3)
+        expect(ring[1]).to eq(4)
+        expect(ring[2]).to eq(5)
+
+        expect(ring[-1]).to eq(5)
+        expect(ring[-2]).to eq(4)
+        expect(ring[-3]).to eq(3)
+      end
+
+      it "reads elements via inclusive range" do
+        expect(ring[1..2]).to eq([4, 5])
+        expect(ring[-2..-1]).to eq([4, 5])
+        expect(ring[-2..3]).to eq([4, 5])
+
+        expect(ring[0..-1]).to eq([3, 4, 5])
+
+        expect(ring[2..-1]).to eq([5])
+        expect(ring[-1..10]).to eq([5])
+
+        expect(ring[-1..0]).to eq([])
+        expect(ring[-1..1]).to eq([])
+      end
+
+      it "reads elements via exclusive range" do
+        expect(ring[1...2]).to eq([4])
+        expect(ring[-2...-1]).to eq([4])
+        expect(ring[-2...3]).to eq([4, 5])
+
+        expect(ring[0...-1]).to eq([3, 4])
+
+        expect(ring[2...-1]).to eq([])
+        expect(ring[-1...10]).to eq([5])
+
+        expect(ring[-1...0]).to eq([])
+        expect(ring[-1...1]).to eq([])
+      end
+    end
   end
 
-  it 'should be able to be added objects to' do
-    expect(@populated.size).to eq 4
-    expect(@populated.to_a).to eq [1, 2, 3, 4]
+  describe "#to_a" do
+    it "returns a duplicate of internal buffer" do
+      array = ring.to_a
+      ring << 1
+      expect(array.count).to eq(0)
+      expect(ring.count).to eq(1)
+    end
   end
 
-  it 'should be able to access single elements' do
-    expect(@populated[2]).to eq 3
-  end
+  describe "#clear" do
+    it "resets ring to initial state" do
+      ring << 1
+      expect(ring.count).to eq(1)
+      expect(ring.to_a).to eq([1])
 
-  it 'should be able to access negative indices' do
-    expect(@populated[-1]).to eq 4
-  end
-
-  it 'should be able to access ranges' do
-    expect(@populated[1..2]).to eq [2, 3]
-  end
-
-  it 'should be able to access ranges starting from a negative index' do
-    expect(@populated[-2..3]).to eq [3, 4]
-  end
-
-  it 'should be able to access ranges ending at a negative index' do
-    expect(@populated[2..-1]).to eq [3, 4]
-  end
-
-  it 'should be able to access ranges using only negative indices' do
-    expect(@populated[-2..-1]).to eq [3, 4]
-  end
-
-  it 'should be able to use range where end is excluded' do
-    expect(@populated[-2...-1]).to eq [3]
-  end
-
-  it 'should be able to access slices using a size' do
-    expect(@populated[-3, 2]).to eq [2, 3]
-  end
-
-  it 'should remove older entries' do
-    11.times { |n| @ring << n }
-
-    expect(@ring[0]).to  eq nil
-    expect(@ring[1]).to  eq 1
-    expect(@ring[10]).to eq 10
-  end
-
-  it 'should not be larger than specified maximum size' do
-    12.times { |n| @ring << n }
-    expect(@ring.entries.compact.size).to eq 10
-  end
-
-  it 'should pop!' do
-    @populated.pop!
-    expect(@populated.to_a).to eq [1, 2, 3]
-  end
-
-  it 'should return an indexed hash' do
-    expect(@populated.to_h[0]).to eq @populated[0]
+      ring.clear
+      expect(ring.count).to eq(0)
+      expect(ring.to_a).to eq([])
+    end
   end
 end