Applied MR review suggestions

lib/gitlab/ci/variables/collection.rb

@@ -10,18 +10,18 @@ module Gitlab
 
         def initialize(variables = [], errors = nil)
           @variables = []
-          @var_hash = {}
+          @variables_by_key = {}
           @errors = errors
 
           variables.each { |variable| self.append(variable) }
         end
 
         def append(resource)
-          tap do
-            item = Collection::Item.fabricate(resource)
-            @variables.append(item)
-            @var_hash[item[:key]] = item
-          end
+          item = Collection::Item.fabricate(resource)
+          @variables.append(item)
+          @variables_by_key[item[:key]] = item
+
+          self
         end
 
         def concat(resources)
@@ -42,7 +42,7 @@ module Gitlab
         end
 
         def [](key)
-          @var_hash[key]
+          @variables_by_key[key]
         end
 
         def size
@@ -53,14 +53,6 @@ module Gitlab
           self.map(&:to_runner_variable)
         end
 
-        def include?(obj)
-          return false unless obj.is_a?(Hash) && obj.has_key?(:key)
-
-          key = obj.fetch(:key)
-          found_var = @var_hash[key]
-          !found_var.nil? && found_var.to_runner_variable == Collection::Item.fabricate(obj).to_runner_variable
-        end
-
         def to_hash
           self.to_runner_variables
             .map { |env| [env.fetch(:key), env.fetch(:value)] }

lib/gitlab/ci/variables/collection/item.rb

@@ -14,6 +14,10 @@ module Gitlab
             }
           end
 
+          def value
+            @variable.fetch(:value)
+          end
+
           def [](key)
             @variable.fetch(key)
           end

lib/gitlab/ci/variables/collection/sort.rb

@@ -52,7 +52,7 @@ module Gitlab
           end
 
           def tsort_each_child(variable, &block)
-            each_variable_reference(variable[:value], &block)
+            each_variable_reference(variable.value, &block)
           end
 
           def input_vars

spec/lib/gitlab/ci/variables/collection/item_spec.rb

@@ -32,6 +32,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Item do
       it 'saves given value' do
         expect(subject[:key]).to eq variable_key
         expect(subject[:value]).to eq expected_value
+        expect(subject.value).to eq expected_value
       end
     end
 

spec/lib/gitlab/ci/variables/collection_spec.rb

@@ -129,26 +129,16 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
 
       expect(collection.size).to eq(2)
     end
-  end
-
-  describe '#include?' do
-    it 'returns false for unknown variable' do
-      collection = described_class.new([])
-
-      expect(collection.include?({ key: 'VAR', value: 'value', public: true, masked: false })).to eq(false)
-    end
-
-    it 'returns false for unsupported scenario of variable name argument' do
-      collection = described_class.new([{ key: 'VAR', value: 'value', public: true, masked: false }])
-
-      expect(collection.include?('VAR')).to eq(false)
-    end
 
-    it 'returns true for known variable' do
-      variable = { key: 'VAR', value: 'value', public: true, masked: false }
-      collection = described_class.new([variable])
+    it 'returns 3 for collection with 2 duplicate variables' do
+      collection = described_class.new(
+        [
+          { key: 'VAR1', value: 'value', public: true, masked: false },
+          { key: 'VAR2', value: 'value', public: true, masked: false },
+          { key: 'VAR1', value: 'value', public: true, masked: false }
+        ])
 
-      expect(collection.include?(variable)).to eq(true)
+      expect(collection.size).to eq(3)
     end
   end