Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Inhibit warnings per single pod #10

Merged
merged 3 commits into from

4 participants

@supermarin
Collaborator

Finally - thanks to @irrationalfab 's support :)

@supermarin supermarin referenced this pull request in CocoaPods/CocoaPods
Merged

added inhibit warnings per pod. fixes #674 #934

@coveralls

Coverage remained the same when pulling 5991d05 on mneorr:inhibit_warnings into a139f44 on CocoaPods:master.

View Details

lib/cocoapods-core/podfile/target_definition.rb
@@ -355,6 +373,7 @@ def set_platform(name, target = nil)
#
def store_pod(name, *requirements)
if requirements && !requirements.empty?
+ inhibit_warnings_if_asked(name, requirements)
@fabiopelosin Owner

Alternative name proposal: process_warning_inhibition

@supermarin Collaborator

or parse_inhibit_warnings ?

@fabiopelosin Owner

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/cocoapods-core/podfile/target_definition.rb
@@ -274,8 +274,15 @@ def inhibit_all_warnings?
get_hash_value('inhibit_all_warnings') || (parent.inhibit_all_warnings? unless root?)
end
+ # @return [Bool] whether the target definition should inhibit warnings
+ # for a single pod
@fabiopelosin Owner

To respect the rest the style of the repo can you indent this line to the level of the [, capitalize and add proper punctuation… Sorry I'm a comments nazi :smile:

@supermarin Collaborator

sure :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/cocoapods-core/podfile/target_definition.rb
@@ -547,6 +564,27 @@ def podspec_path_from_options(options)
end
end
+ # Removes :inhibit_warnings from the requirements list, and adds
+ # the pod's name into internal hash for disabling warnings.
+ #
+ # @param [String] pod name
+ #
+ # @param [Array] requirements. If :inhibit_warnings is the only key
+ # in the hash, the hash should be destroyed because it
+ # confuses Gem::Dependency. (it thinks it's an ext. source)
+ #
+ # @return [void]
+ #
+ def inhibit_warnings_if_asked(name, requirements)
@fabiopelosin Owner

I would prefer not to rely in the side effect of deleting the key of the hash. For purity and given the small size of the argument, I would dup it, delete the key, and return that as a result.

@supermarin Collaborator

Correct, but then you end up with something like [:head, {}], or ['>0' , {}] or even [{}] .
When you instantiate Dependency with Dependency.new('foo', {}) it breaks because it thinks you gave it something in the hash.

The other thing - returning array instead of modifying the original one, that sounds ok

@fabiopelosin Owner

I was thinking about something like dep_requirements = inhibit_warnings_if_asked(name, requirements)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/cocoapods-core/podfile/target_definition.rb
@@ -547,6 +564,27 @@ def podspec_path_from_options(options)
end
end
+ # Removes :inhibit_warnings from the requirements list, and adds
+ # the pod's name into internal hash for disabling warnings.
+ #
+ # @param [String] pod name
+ #
+ # @param [Array] requirements. If :inhibit_warnings is the only key
@fabiopelosin Owner

Comments nazi back again, YARD would be confused by the dot. And in the rest of the codebase the description is added in a line below aligned with the [.

@supermarin Collaborator

sorry about the wrong indentation, life lesson learned. :smile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabiopelosin fabiopelosin commented on the diff
lib/cocoapods-core/podfile/target_definition.rb
@@ -407,6 +423,7 @@ def store_podspec(options = nil)
'link_with',
'link_with_first_target',
'inhibit_all_warnings',
+ "inhibited_warnings_per_pod",
@fabiopelosin Owner

What about storing everything in a hash in the inhibit_all_warnings key. Similar to what thepodspec does? In my idea this hash could have two keys all (which takes a bool) or names (which takes an array).

@supermarin Collaborator

Probably that's the same one from the above :)

Whatever gets decided, I'm fine with both.

@fabiopelosin Owner

I think that the later is better for the YAML but I would rename it to inhibit_warnings

---
target_definitions:
- name: Pods
  link_with_first_target: true
  inhibit_warnings:
    all_pods: true
    names: 'AFNetworking'
  dependencies:
  - ASIHTTPRequest
  - JSONKit:
    - '> 1.0'
generate_bridge_support: true
set_arc_compatibility_flag: true

What do you think?

/c @alloy

@alloy Owner
alloy added a note

If we’re going for pretty, I would rename all_pods to all. Other than that it looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/cocoapods-core/podfile/target_definition.rb
@@ -390,9 +409,6 @@ def store_podspec(options = nil)
end
end
- #-----------------------------------------------------------------------#
@fabiopelosin Owner

Can you restore these comments one for consistency with the rest of the repo?

@supermarin Collaborator

I've thought the line was used to separate public from private, and accidentally there was 2 public keywords so I removed both.

  • turned back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabiopelosin fabiopelosin commented on the diff
lib/cocoapods-core/podfile/target_definition.rb
((11 lines not shown))
# Sets whether the target definition should inhibit the warnings during
- # compilation.
+ # compilation for all pods.
#
# @param [Bool] flag
# Whether the warnings should be suppressed.
@fabiopelosin Owner

Do you think that it would be a bad idea to merge this method in #inhibit_warnings_for_pod and remove it. In this way in the CP gem we could only do the check once. This as the disadvantage that instead of having a single compiler flag for inhibit all warnings it would add one for every build file (although I don't see any issue with it).

@supermarin Collaborator

Hm, not sure how I feel about that; @alloy what are your thoughts?

One of the advantages of keeping it is - we can add a small optimization.
If inhibit_all_warnings is true, we don't have to check and add stuff per pod at all. So speed vs less code :)

@fabiopelosin Owner

I think that this computation is too small to affect the execution speed.

@fabiopelosin Owner

For the record, After discussing with Eloy, the agreement is that wen't have any strong opinion about it. In this case the best solution might be to merge the methods as the simplified logic is easier to maintain.

@fabiopelosin Owner

CocoaPods/CocoaPods#821 is a good reason to have all the settings added to the build files.

@supermarin Collaborator

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coveralls

Coverage remained the same when pulling 2d8a638 on mneorr:inhibit_warnings into a139f44 on CocoaPods:master.

View Details

lib/cocoapods-core/podfile/target_definition.rb
@@ -392,8 +411,6 @@ def store_podspec(options = nil)
#-----------------------------------------------------------------------#
- public
@fabiopelosin Owner

I see what you mean. The style that I was following is to open every new YARDoc group with the line to separate it, and then indicate the visibility regardless of the one of the previous group.

@supermarin Collaborator

oh.. got it. do you want me to force-push? :smile_cat:

@fabiopelosin Owner

:laughing:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coveralls

Coverage remained the same when pulling 4966d05 on mneorr:inhibit_warnings into eb67003 on CocoaPods:master.

View Details

@supermarin supermarin merged commit 17179bd into CocoaPods:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
43 lib/cocoapods-core/podfile/target_definition.rb
@@ -274,8 +274,15 @@ def inhibit_all_warnings?
get_hash_value('inhibit_all_warnings') || (parent.inhibit_all_warnings? unless root?)
end
+ # @return [Bool] whether the target definition should inhibit warnings
+ # for a single pod
+ #
+ def inhibits_warnings_for_pod?(pod_name)
+ get_hash_value("inhibited_warnings_per_pod", []).include? pod_name
+ end
+
# Sets whether the target definition should inhibit the warnings during
- # compilation.
+ # compilation for all pods.
#
# @param [Bool] flag
# Whether the warnings should be suppressed.
@fabiopelosin Owner

Do you think that it would be a bad idea to merge this method in #inhibit_warnings_for_pod and remove it. In this way in the CP gem we could only do the check once. This as the disadvantage that instead of having a single compiler flag for inhibit all warnings it would add one for every build file (although I don't see any issue with it).

@supermarin Collaborator

Hm, not sure how I feel about that; @alloy what are your thoughts?

One of the advantages of keeping it is - we can add a small optimization.
If inhibit_all_warnings is true, we don't have to check and add stuff per pod at all. So speed vs less code :)

@fabiopelosin Owner

I think that this computation is too small to affect the execution speed.

@fabiopelosin Owner

For the record, After discussing with Eloy, the agreement is that wen't have any strong opinion about it. In this case the best solution might be to merge the methods as the simplified logic is easier to maintain.

@fabiopelosin Owner

CocoaPods/CocoaPods#821 is a good reason to have all the settings added to the build files.

@supermarin Collaborator

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@@ -286,6 +293,17 @@ def inhibit_all_warnings=(flag)
set_hash_value('inhibit_all_warnings', flag)
end
+ # Inhibits warnings for a specific pod during compilation.
+ #
+ # @param [String] pod name
+ # Whether the warnings should be suppressed.
+ #
+ # @return [void]
+ #
+ def inhibit_warnings_for_pod(pod_name)
+ get_hash_value('inhibited_warnings_per_pod', []) << pod_name
+ end
+
#--------------------------------------#
# @return [Platform] the platform of the target definition.
@@ -355,6 +373,7 @@ def set_platform(name, target = nil)
#
def store_pod(name, *requirements)
if requirements && !requirements.empty?
+ parse_inhibit_warnings(name, requirements)
pod = { name => requirements }
else
pod = name
@@ -407,6 +426,7 @@ def store_podspec(options = nil)
'link_with',
'link_with_first_target',
'inhibit_all_warnings',
+ "inhibited_warnings_per_pod",
@fabiopelosin Owner

What about storing everything in a hash in the inhibit_all_warnings key. Similar to what thepodspec does? In my idea this hash could have two keys all (which takes a bool) or names (which takes an array).

@supermarin Collaborator

Probably that's the same one from the above :)

Whatever gets decided, I'm fine with both.

@fabiopelosin Owner

I think that the later is better for the YAML but I would rename it to inhibit_warnings

---
target_definitions:
- name: Pods
  link_with_first_target: true
  inhibit_warnings:
    all_pods: true
    names: 'AFNetworking'
  dependencies:
  - ASIHTTPRequest
  - JSONKit:
    - '> 1.0'
generate_bridge_support: true
set_arc_compatibility_flag: true

What do you think?

/c @alloy

@alloy Owner
alloy added a note

If we’re going for pretty, I would rename all_pods to all. Other than that it looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
'user_project_path',
'build_configurations',
'dependencies',
@@ -547,6 +567,27 @@ def podspec_path_from_options(options)
end
end
+ # Removes :inhibit_warnings from the requirements list, and adds
+ # the pod's name into internal hash for disabling warnings.
+ #
+ # @param [String] pod name
+ #
+ # @param [Array] requirements
+ # If :inhibit_warnings is the only key in the hash, the hash
+ # should be destroyed because it confuses Gem::Dependency.
+ #
+ # @return [void]
+ #
+ def parse_inhibit_warnings(name, requirements)
+ options = requirements.last
+ return requirements unless options.is_a?(Hash)
+
+ should_inhibit = options.delete(:inhibit_warnings)
+ inhibit_warnings_for_pod(name) if should_inhibit
+
+ requirements.pop if options.empty?
+ end
+
#-----------------------------------------------------------------------#
end
View
16 spec/podfile/target_definition_spec.rb
@@ -195,6 +195,22 @@ module Pod
@root.should.not.inhibit_all_warnings?
end
+ it "doesn't inhibit warnings per pod by default" do
+ @root.store_pod("ObjectiveSugar")
+ @root.should.not.inhibits_warnings_for_pod?("ObjectiveSugar")
+ end
+
+ it "inhibits warnings per pod if passed to store_pod" do
+ @root.store_pod("Objective-Record", :head, :inhibit_warnings => true)
+ @root.should.inhibits_warnings_for_pod?("Objective-Record")
+ end
+
+ it "must delete the hash if it was empty. otherwise breaks Dependency" do
+ reqs = [{ :inhibit_warnings => true }]
+ @root.send(:parse_inhibit_warnings, 'Objective-Record', reqs)
+ reqs.should.be.empty?
+ end
+
it "returns if it should inhibit all warnings" do
@root.inhibit_all_warnings = true
@root.should.inhibit_all_warnings?
Something went wrong with that request. Please try again.