From e50020ea9ebbfdd87c24677048439108991079f4 Mon Sep 17 00:00:00 2001 From: Justin Martin Date: Tue, 27 Oct 2015 02:00:21 -0700 Subject: [PATCH] PR feedback --- lib/xcodeproj/scheme/environment_variables.rb | 88 ++++++++++++------- lib/xcodeproj/scheme/launch_action.rb | 20 ++--- lib/xcodeproj/scheme/test_action.rb | 21 ++--- spec/scheme/environment_variables_spec.rb | 32 +++---- spec/scheme/launch_action_spec.rb | 6 +- spec/scheme/test_action_spec.rb | 6 +- 6 files changed, 95 insertions(+), 78 deletions(-) diff --git a/lib/xcodeproj/scheme/environment_variables.rb b/lib/xcodeproj/scheme/environment_variables.rb index f6858a9ec..54b93b84c 100644 --- a/lib/xcodeproj/scheme/environment_variables.rb +++ b/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,Array 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 => , :value => (, :enabled => )}. + # @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] # 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 => , :value => (, :enabled => )}. + # @see EnvironmentVariable#initialize + # @return [Array] # 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] # 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 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 => , :value => (, :enabled => )}. # - 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 diff --git a/lib/xcodeproj/scheme/launch_action.rb b/lib/xcodeproj/scheme/launch_action.rb index 6e51502c9..e54885a29 100644 --- a/lib/xcodeproj/scheme/launch_action.rb +++ b/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 diff --git a/lib/xcodeproj/scheme/test_action.rb b/lib/xcodeproj/scheme/test_action.rb index 01566c999..87f3bd643 100644 --- a/lib/xcodeproj/scheme/test_action.rb +++ b/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 #-------------------------------------------------------------------------# diff --git a/spec/scheme/environment_variables_spec.rb b/spec/scheme/environment_variables_spec.rb index 0a34a069a..040ad874e 100644 --- a/spec/scheme/environment_variables_spec.rb +++ b/spec/scheme/environment_variables_spec.rb @@ -57,13 +57,13 @@ 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 == "" end @@ -71,41 +71,41 @@ def self.xml_for_object(object) 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 == "" @@ -130,9 +130,9 @@ def self.xml_for_object(object) Xcodeproj.xml_for_object(subject).should == '' 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 diff --git a/spec/scheme/launch_action_spec.rb b/spec/scheme/launch_action_spec.rb index 70237f0e5..0507d70a3 100644 --- a/spec/scheme/launch_action_spec.rb +++ b/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 diff --git a/spec/scheme/test_action_spec.rb b/spec/scheme/test_action_spec.rb index c1d635017..f78805806 100644 --- a/spec/scheme/test_action_spec.rb +++ b/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