Skip to content

Add validation to check that a podspec's name doesn't contain whitespace #39

Merged
merged 4 commits into from Nov 28, 2013

4 participants

@Kapin
CocoaPods member
Kapin commented Nov 19, 2013

If we want to go ahead with CocoaPods/CocoaPods#1610 this will add the correct validation for it. The test probably could be a bit cleaner but I was fighting with it to avoid the name mismatch error too.

@coveralls

Coverage Status

Coverage decreased (-0.24%) when pulling 2f06d53 on Kapin:whitespace-fix into 5212432 on CocoaPods:master.

@alloy alloy and 1 other commented on an outdated diff Nov 19, 2013
spec/specification/linter_spec.rb
@@ -135,6 +135,13 @@ def message_should_include(*values)
message_should_include('name', 'match')
end
+ it "fails a specification whose name contains whitespace" do
+ @spec.stubs(:name).returns('bad name')
+ @linter.lint
+ @linter.results.count.should == 2
@alloy
CocoaPods member
alloy added a note Nov 19, 2013

Why is it important to verify that there are 2 errors?

@alloy
CocoaPods member
alloy added a note Nov 19, 2013

Ok, I see where the second error comes from, namely the name not matching the file name? I see two alternative solutions:

  • Stub the basename attribute of the file object, e.g. @linter.file.stubs(:basename).returns('ValidName'), which is used here.
  • Modify the message_should_include test helper to find any matching message among the results, making it index independent.

I think I hinge towards the latter.

@Kapin
CocoaPods member
Kapin added a note Nov 19, 2013

@alloy I definitely agree with the latter.

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

Coverage Status

Coverage decreased (-0.24%) when pulling 5d9dbb2 on Kapin:whitespace-fix into 5212432 on CocoaPods:master.

@alloy alloy commented on an outdated diff Nov 22, 2013
spec/specification/linter_spec.rb
@@ -95,10 +95,14 @@ module Pod
def message_should_include(*values)
@linter.lint
- result = @linter.results.first
- result.should.not.be.nil
- @linter.results.map(&:message).should == [result.message]
- message = result.message.downcase
+ results = @linter.results
+ results.should.not.be.nil
+
+ results.each do |result|
+ results.map(&:message).should.include(result.message)
+ end
@alloy
CocoaPods member
alloy added a note Nov 22, 2013

This only verifies that all the mapped messages are the same as where they originated from (the result).

You probably want to do something like:

matched = results.select do |result|
  values.all? do |value|
    result.message.include?(value.downcase)
  end
end
matched.size.should == 1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coveralls

Coverage Status

Coverage decreased (-0.2%) when pulling 3a21f32 on Kapin:whitespace-fix into 5212432 on CocoaPods:master.

@alloy alloy and 2 others commented on an outdated diff Nov 23, 2013
spec/specification/linter_spec.rb
end
+
+ matched.size.should >= 1
@alloy
CocoaPods member
alloy added a note Nov 23, 2013

Should the matched size not simply match the values size? With the current condition, you can’t know for sure that the values you expected are all there.

@fabiopelosin
CocoaPods member
fabiopelosin added a note Nov 27, 2013

I think @alloy's point makes a lot of sense. Do the other tests still pass with this change?

@Kapin
CocoaPods member
Kapin added a note Nov 27, 2013

Two things I saw. When just checking for 1 I had two errors generated (when they should be) and it caused an overlap. Using all? instead resulted in other tests failing. I'm going to look back into the all option because it made more sense and see why those other tests were having issues.

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

👍

@coveralls

Coverage Status

Coverage decreased (-0.24%) when pulling 957fedb on Kapin:whitespace-fix into 5212432 on CocoaPods:master.

@fabiopelosin
CocoaPods member

@Kapin you commented that using all? would make tests failing, however they passed without any other change. am I missing something? Otherwise can we merge this patch?

Finally once merged could you add an entry to the CocoaPods changelog? Although is a minor change I would prefer to document it.

@Kapin
CocoaPods member
Kapin commented Nov 28, 2013

@irrationalfab I had a complete brainfart and forgot to downcase the message I was comparing. Therefore it was failing when Example was in the string but not example.

@Kapin Kapin merged commit 187b28b into CocoaPods:master Nov 28, 2013

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.