PR feedback

lib/xcodeproj/scheme/environment_variables.rb

@@ -10,52 +10,60 @@ class XCScheme
     # LaunchAction or TestAction scheme group.
     #
     class EnvironmentVariables < XMLElementWrapper
-      # @param [REXML::Element,Array{EnvironmentVariable},Array{Hash{String => String}}] node_or_target
-      #   The 'EnvironmentVariables' XML node that this object represents.
-      #   If nil, it will create a default XML node to use.
+      # @param [REXML::Element,Array<EnvironmentVariable>,Array<Hash{Symbol => String,Bool}>] node_or_variables
+      #   The 'EnvironmentVariables' XML node that this object represents. If nil, it will create a default XML
+      #   node to use.
+      # @note If initializing with an array of hashes, the hash should be of the form
+      #   {:key => <String>, :value => <String>(, :enabled => <Bool>)}.
+      # @see EnvironmentVariable#initialize
       #
-      def initialize(node_or_target = nil)
-        create_xml_element_with_fallback(node_or_target, VARIABLES_NODE) do
+      def initialize(node_or_variables = nil)
+        create_xml_element_with_fallback(node_or_variables, VARIABLES_NODE) do
           @all_variables = []
-          node_or_target.each { |var| add_variable(var) } unless node_or_target.nil?
+          node_or_variables.each { |var| upsert_variable(var) } unless node_or_variables.nil?
         end
       end
 
-      # @return [Array{EnvironmentVariable}]
+      # @return [Array<EnvironmentVariable>]
       #   The key value pairs currently set in @xml_element
       #
       def all_variables
         @all_variables ||= @xml_element.get_elements(VARIABLE_NODE).map { |variable| EnvironmentVariable.new(variable) }
       end
 
-      # @param [EnvironmentVariable,Hash{String => String}] variable
-      #   The key value pairs currently set in @xml_element
+      # Adds a given variable to the set of environment variables, or replaces it if that key already exists
       #
-      # @return [Array{EnvironmentVariable}]
+      # @param [EnvironmentVariable,Hash{Symbol => String,Bool}] variable
+      #   The variable to set
+      # @note If initializing with an array of hashes, the hash should be of the form
+      #   {:key => <String>, :value => <String>(, :enabled => <Bool>)}.
+      # @see EnvironmentVariable#initialize
+      # @return [Array<EnvironmentVariable>]
       #   The new set of environment variables after addition
       #
-      def add_variable(variable)
+      def upsert_variable(variable)
         env_var = variable.is_a?(EnvironmentVariable) ? variable : EnvironmentVariable.new(variable)
         all_variables.each { |existing_var| remove_variable(existing_var) if existing_var.key == env_var.key }
         @xml_element.add_element(env_var.xml_element)
         @all_variables << env_var
       end
 
-      # @param [EnvironmentVariable,Hash{String => String}] variable
-      #   The key value pairs currently set in @xml_element
+      # Removes a specified variable (by string or object) from the set of environment variables
       #
-      # @return [Array{EnvironmentVariable}]
+      # @param [EnvironmentVariable,String] variable
+      #   The variable to remove
+      # @return [Array<EnvironmentVariable>]
       #   The new set of environment variables after removal
       #
       def remove_variable(variable)
-        env_var = variable.is_a?(EnvironmentVariable) ? variable : all_variables.select { |var| var.key == variable[:key] }[0]
+        env_var = variable.is_a?(EnvironmentVariable) ? variable : all_variables.find { |var| var.key == variable }
         raise "Unexpected parameter type: #{env_var.class}" unless env_var.is_a?(EnvironmentVariable)
         @xml_element.delete_element(env_var.xml_element)
         @all_variables -= [env_var]
       end
 
-      # @param [EnvironmentVariable,Hash{String => String}] variable
-      #   The key value pairs currently set in @xml_element
+      # @return Array<Hash{Symbol => String,Bool}>
+      #   The current environment variables represented as an array
       #
       def to_a
         all_variables.map(&:to_h)
@@ -67,48 +75,68 @@ def to_a
     # [[NSProcessInfo processInfo] environment] in your app code.
     #
     class EnvironmentVariable < XMLElementWrapper
-      # @param [REXML::Element,EnvironmentVariable,Hash{String => String}] value
-      #   The 'EnvironmentVariable' XML node that this object represents.
-      #   If nil, it will create a default XML node to use. If a hash, it
-      #   must contain keys 'key' and 'value', and optionally have 'enabled'.
-      #   If not supplied, it will default to being enabled.
+      # @param [REXML::Element,Hash{Symbol => String,Bool}] value
+      #   The 'EnvironmentVariable' XML node that this object represents. If nil,
+      #   it will create a default XML node to use. If a hash, it must contain
+      #   keys 'key' and 'value' with string values, and optionally have 'enabled'
+      #   as a boolean. If 'enabled' is not supplied, it will default to true.
+      # @note If initializing with a hash, it should be of the form
+      #   {:key => <String>, :value => <String>(, :enabled => <Bool>)}.
       #
-      def initialize(value)
-        create_xml_element_with_fallback(value, VARIABLE_NODE) do
-          raise "Must pass a Hash with 'key' and 'value'!" unless value.is_a?(Hash) && value.key?(:key) && value.key?(:value)
+      def initialize(node_or_variable)
+        create_xml_element_with_fallback(node_or_variable, VARIABLE_NODE) do
+          raise "Must pass a Hash with 'key' and 'value'!" unless node_or_variable.is_a?(Hash) &&
+              node_or_variable.key?(:key) && node_or_variable.key?(:value)
+
+          @xml_element.attributes['key'] = node_or_variable[:key]
+          @xml_element.attributes['value'] = node_or_variable[:value]
 
-          @xml_element.attributes['key'] = value[:key]
-          @xml_element.attributes['value'] = value[:value]
-          @xml_element.attributes['isEnabled'] = value.key?(:enabled) ? bool_to_string(value[:enabled]) : bool_to_string(true)
+          if node_or_variable.key?(:enabled)
+            @xml_element.attributes['isEnabled'] = bool_to_string(node_or_variable[:enabled])
+          else
+            @xml_element.attributes['isEnabled'] = bool_to_string(true)
+          end
         end
       end
 
+      # Returns the EnvironmentValue's key
+      # @return [String]
       def key
         @xml_element.attributes['key']
       end
 
+      # Sets the EnvironmentValue's key
+      # @param [String] key
       def key=(key)
         @xml_element.attributes['key'] = key
       end
 
+      # Returns the EnvironmentValue's value
+      # @return [String]
       def value
         @xml_element.attributes['value']
       end
 
+      # Sets the EnvironmentValue's value
+      # @param [String] value
       def value=(value)
         @xml_element.attributes['value'] = value
       end
 
+      # Returns the EnvironmentValue's enabled state
+      # @return [Bool]
       def enabled
         string_to_bool(@xml_element.attributes['isEnabled'])
       end
 
+      # Sets the EnvironmentValue's enabled state
+      # @param [Bool] enabled
       def enabled=(enabled)
         @xml_element.attributes['isEnabled'] = bool_to_string(enabled)
       end
 
-      # @return [Hash{:key => String, :value => String, :enabled => Boolean}]
-      #   The environment variable XML node with attributes converted to Hash keys
+      # @return [Hash{:key => String, :value => String, :enabled => Bool}]
+      #   The environment variable XML node with attributes converted to a representative Hash
       def to_h
         { :key => key, :value => value, :enabled => enabled }
       end

lib/xcodeproj/scheme/launch_action.rb

@@ -63,25 +63,19 @@ def buildable_product_runnable=(runnable)
       end
 
       # @return [EnvironmentVariables]
-      #        Returns the set of environment variables that are currently set at test runtime
-      #        NOTE: If this creates a new node, it will not automatically be added to the xml document
+      #        Returns the EnvironmentVariables that should exist at app launch
       #
       def environment_variables
-        env_vars_nodes = @xml_element.get_elements(XCScheme::VARIABLES_NODE)
-        raise 'Unexpected number of EnvironmentVariables nodes found!' if env_vars_nodes.count > 1
-
-        env_vars_nodes.count == 0 ? nil : EnvironmentVariables.new(env_vars_nodes[0])
+        EnvironmentVariables.new(@xml_element.elements[XCScheme::VARIABLES_NODE])
       end
 
-      # @param [EnvironmentVariables] env_vars
-      #        Overwrites the set of environment variables that should be set at test runtime
+      # @param [EnvironmentVariables,nil] env_vars
+      #        Sets the EnvironmentVariables that should exist at app launch
       #
       def environment_variables=(env_vars)
-        @xml_element.get_elements(XCScheme::VARIABLES_NODE).each { |node| node.parent.delete_element(node) }
-
-        raise "Unexpected parameter type: #{env_vars.class}" unless env_vars.nil? || env_vars.is_a?(EnvironmentVariables)
-        @xml_element.add_element(env_vars.xml_element) unless env_vars.nil?
-        environment_variables
+        @xml_element.delete_element(XCScheme::VARIABLES_NODE)
+        @xml_element.add_element(env_vars.xml_element) if env_vars
+        env_vars
       end
 
       # @todo handle 'AdditionalOptions' tag

lib/xcodeproj/scheme/test_action.rb

@@ -86,25 +86,20 @@ def add_macro_expansion(macro_expansion)
       end
 
       # @return [EnvironmentVariables]
-      #        Returns the set of environment variables that are currently set at test runtime
-      #        NOTE: If this creates a new node, it will not automatically be added to the xml document
+      #        Returns the EnvironmentVariables that should exist at test launch
       #
       def environment_variables
-        env_vars_nodes = @xml_element.get_elements(XCScheme::VARIABLES_NODE)
-        raise 'Unexpected number of EnvironmentVariables nodes found!' if env_vars_nodes.count > 1
-
-        env_vars_nodes.count == 0 ? nil : EnvironmentVariables.new(env_vars_nodes[0])
+        EnvironmentVariables.new(@xml_element.elements[XCScheme::VARIABLES_NODE])
       end
 
-      # @param [EnvironmentVariables] env_vars
-      #        Overwrites the set of environment variables that should be set at test runtime
+      # @param [EnvironmentVariables,nil] env_vars
+      #        Sets the EnvironmentVariables that should exist at test launch
+      # @return [EnvironmentVariables]
       #
       def environment_variables=(env_vars)
-        @xml_element.get_elements(XCScheme::VARIABLES_NODE).each { |node| node.parent.delete_element(node) }
-
-        raise "Unexpected parameter type: #{env_vars.class}" unless env_vars.nil? || env_vars.is_a?(EnvironmentVariables)
-        @xml_element.add_element(env_vars.xml_element) unless env_vars.nil?
-        environment_variables
+        @xml_element.delete_element(XCScheme::VARIABLES_NODE)
+        @xml_element.add_element(env_vars.xml_element) if env_vars
+        env_vars
       end
 
       #-------------------------------------------------------------------------#

spec/scheme/environment_variables_spec.rb

@@ -57,55 +57,55 @@ def self.xml_for_object(object)
       end
     end
 
-    describe '#add_variable' do
+    describe '#upsert_variable' do
       it 'adds a new environment variable by Hash' do
         subject = XCScheme::EnvironmentVariables.new
-        subject.add_variable(:key => 'key1', :value => 'value1')
+        subject.upsert_variable(:key => 'key1', :value => 'value1')
 
         subject.all_variables.count.should == 1
-        subject.all_variables[0].to_h.should == { :key => 'key1', :value => 'value1', :enabled => true }
+        subject.all_variables.first.to_h.should == { :key => 'key1', :value => 'value1', :enabled => true }
         Xcodeproj.xml_for_object(subject).should ==
           "<EnvironmentVariables><EnvironmentVariable key='key1' value='value1' isEnabled='YES'/></EnvironmentVariables>"
       end
 
       it 'adds a new variable by object using the same object' do
         subject = XCScheme::EnvironmentVariables.new
         new_var = XCScheme::EnvironmentVariable.new(:key => 'key1', :value => 'value1')
-        subject.add_variable(new_var)
+        subject.upsert_variable(new_var)
 
         subject.all_variables.count.should == 1
-        subject.all_variables[0].to_h.should == { :key => 'key1', :value => 'value1', :enabled => true }
-        subject.all_variables[0].should.equal?(new_var)
+        subject.all_variables.first.to_h.should == { :key => 'key1', :value => 'value1', :enabled => true }
+        subject.all_variables.first.should.equal?(new_var)
       end
 
       it 'adds a new variable by node using the same xml_element' do
         subject = XCScheme::EnvironmentVariables.new
         new_var = XCScheme::EnvironmentVariable.new(:key => 'key1', :value => 'value1')
-        subject.add_variable(new_var.xml_element)
+        subject.upsert_variable(new_var.xml_element)
 
         subject.all_variables.count.should == 1
-        subject.all_variables[0].to_h.should == { :key => 'key1', :value => 'value1', :enabled => true }
-        subject.all_variables[0].xml_element.should.equal?(new_var.xml_element)
+        subject.all_variables.first.to_h.should == { :key => 'key1', :value => 'value1', :enabled => true }
+        subject.all_variables.first.xml_element.should.equal?(new_var.xml_element)
       end
 
       it 'merges in a new entry if a matching key does not exist' do
         subject = XCScheme::EnvironmentVariables.new([:key => 'key1', :value => 'value1'])
-        subject.add_variable(:key => 'key2', :value => 'value2')
+        subject.upsert_variable(:key => 'key2', :value => 'value2')
         subject.to_a.should == [{ :key => 'key1', :value => 'value1', :enabled => true },
                                 { :key => 'key2', :value => 'value2', :enabled => true }]
       end
 
       it 'updates an existing variable value if one already exists with that key' do
         subject = XCScheme::EnvironmentVariables.new([:key => 'key1', :value => 'value1'])
-        subject.add_variable(:key => 'key1', :value => 'value3')
+        subject.upsert_variable(:key => 'key1', :value => 'value3')
         subject.to_a.should == [:key => 'key1', :value => 'value3', :enabled => true]
       end
 
       it 'updates an existing variable enabled state if one already exists with that key' do
         subject = XCScheme::EnvironmentVariables.new([{ :key => 'key1', :value => 'value1' },
                                                       { :key => 'key2', :value => 'value2', :enabled => false }])
-        subject.add_variable(:key => 'key1', :value => 'value1', :enabled => false)
-        subject.add_variable(:key => 'key2', :value => 'value2')
+        subject.upsert_variable(:key => 'key1', :value => 'value1', :enabled => false)
+        subject.upsert_variable(:key => 'key2', :value => 'value2')
 
         subject.to_a.should == [{ :key => 'key1', :value => 'value1', :enabled => false },
                                 { :key => 'key2', :value => 'value2', :enabled => true }]
@@ -116,7 +116,7 @@ def self.xml_for_object(object)
       it 'returns the new set of environment variables after removal' do
         subject = XCScheme::EnvironmentVariables.new([{ :key => 'key1', :value => 'value1' },
                                                       { :key => 'key2', :value => 'value2' }])
-        subject.remove_variable(:key => 'key1')[0].to_h.should == { :key => 'key2', :value => 'value2', :enabled => true }
+        subject.remove_variable('key1').first.to_h.should == { :key => 'key2', :value => 'value2', :enabled => true }
         subject.to_a.should == [:key => 'key2', :value => 'value2', :enabled => true]
         Xcodeproj.xml_for_object(subject).should ==
           "<EnvironmentVariables><EnvironmentVariable key='key2' value='value2' isEnabled='YES'/></EnvironmentVariables>"
@@ -130,9 +130,9 @@ def self.xml_for_object(object)
         Xcodeproj.xml_for_object(subject).should == '<EnvironmentVariables/>'
       end
 
-      it 'removes an existing entry with matching EnvironmentVariable object' do
+      it 'removes an existing entry with matching String' do
         subject = XCScheme::EnvironmentVariables.new([:key => 'key1', :value => 'value1'])
-        subject.remove_variable(:key => 'key1')
+        subject.remove_variable('key1')
         subject.to_a.should == []
       end
 

spec/scheme/launch_action_spec.rb

@@ -62,7 +62,7 @@ module Xcodeproj
 
         it 'starts as nil if no xml exists' do
           @sut.xml_element.elements[vars_node_name].should.equal nil
-          @sut.environment_variables.should.equal nil
+          @sut.environment_variables.to_a.should.equal []
         end
 
         it 'picks up an existing node if exists in the XML' do
@@ -95,8 +95,8 @@ module Xcodeproj
           @sut.xml_element.get_elements(vars_node_name).count.should == 1
 
           @sut.environment_variables = nil
-          @sut.environment_variables.should.equal nil
-          @sut.xml_element.get_elements(vars_node_name).count.should == 0
+          @sut.environment_variables.to_a.should.equal []
+          @sut.xml_element.elements[vars_node_name].should.be.nil
         end
 
         it 'assigning an EnvironmentVariables object updates the xml' do

spec/scheme/test_action_spec.rb

@@ -103,7 +103,7 @@ module Xcodeproj
 
         it 'starts as nil if no xml exists' do
           @sut.xml_element.elements[vars_node_name].should.equal nil
-          @sut.environment_variables.should.equal nil
+          @sut.environment_variables.to_a.should.equal []
         end
 
         it 'picks up an existing node if exists in the XML' do
@@ -136,8 +136,8 @@ module Xcodeproj
           @sut.xml_element.get_elements(vars_node_name).count.should == 1
 
           @sut.environment_variables = nil
-          @sut.environment_variables.should.equal nil
-          @sut.xml_element.get_elements(vars_node_name).count.should == 0
+          @sut.environment_variables.to_a.should.equal []
+          @sut.xml_element.elements[vars_node_name].should.be.nil
         end
 
         it 'assigning an EnvironmentVariables object updates the xml' do