[Linter] check frameworks attribute for invalid frameworks #96

Merged
merged 5 commits into from Apr 5, 2014

Conversation

Projects
None yet
3 participants
Contributor

squarefrog commented Apr 3, 2014

Added regex to fail any frameworks/libraries that allow non-alphanumeric characters. Closes #66

lib/cocoapods-core/specification/linter.rb
@@ -381,7 +382,7 @@ def frameworks_invalid?(frameworks)
# @return [Boolean] true if a library ends with `.a`, `.dylib`, or
@Kapin

Kapin Apr 3, 2014

Member

Can you update the comment here like you did above for frameworks?

+ @spec.frameworks = ["AddressBook, QuartzCore"]
+ message_should_include('framework', 'name')
+ end
+
it 'checks that weak frameworks do not end with a .framework extension' do
@Kapin

Kapin Apr 3, 2014

Member

Might be better to just update this test case. Since the check is no longer for just ending with the .framework parameter and is for any non-alphanumeric characters.

@squarefrog

squarefrog Apr 4, 2014

Contributor

@Kapin This is a test that focuses only on unwanted characters. While not completely obvious - its checking to see what happens when we have a single array item with a string, which could occur when a user forgets a ".

+ @spec.weak_frameworks = ["AddressBook, QuartzCore"]
+ message_should_include('weak framework', 'name')
+ end
+
#------------------#
it 'checks that libraries do not end with a .a extension' do
@Kapin

Kapin Apr 3, 2014

Member

Similar to the previous comment here, might make sense to update this test case.

@fabiopelosin

fabiopelosin Apr 4, 2014

Owner

I would prefer to keep the different tests, because although the code is the same the cases are different (including an extension, from other typo related errors).

Member

Kapin commented Apr 3, 2014

Just a few small comments but awesome job. Can you make sure to add an entry to the CHANGELOG.md file?

lib/cocoapods-core/specification/linter.rb
#
def frameworks_invalid?(frameworks)
- frameworks.any? { |framework| framework.end_with?('.framework') }
+ frameworks.any? { |framework| framework =~ /[^a-zA-Z\d]/ }
@Kapin

Kapin Apr 3, 2014

Member

This may be bad ruby conventions (not the ruby expert here), but it might make sense to make the regex a variable so we can easily modify it we need to add/remove stuff from it.

@squarefrog

squarefrog Apr 4, 2014

Contributor

I think it would help to document the code. E.g.

framework_regex = /[^a-zA-Z\d]/
framework ~= framework_regex

But we'll see how it looks when the old lines are added back in.

lib/cocoapods-core/specification/linter.rb
#
def frameworks_invalid?(frameworks)
- frameworks.any? { |framework| framework.end_with?('.framework') }
@fabiopelosin

fabiopelosin Apr 3, 2014

Owner

This line should not be removed... as this logic is still desiderable

@squarefrog

squarefrog Apr 4, 2014

Contributor

@irrationalfab as below - the reason I removed that line is because the logic should be covered by the regex. I don't believe there is any logical benefit from including the old line (except maybe failing earlier?). But if you'd like it to still be there for documenting purposes it would be no problem for me to fix.

Contributor

squarefrog commented Apr 3, 2014

👍 some great comments there. Ill action them all tomorrow morning.

lib/cocoapods-core/specification/linter.rb
@@ -381,7 +382,7 @@ def frameworks_invalid?(frameworks)
# @return [Boolean] true if a library ends with `.a`, `.dylib`, or
# starts with `lib`.
def libraries_invalid?(libs)
- libs.any? { |lib| lib.end_with?('.a', '.dylib') || lib.start_with?('lib') }
@fabiopelosin

fabiopelosin Apr 3, 2014

Owner

same for this please keep it:

This can be changed to:

libs.any? do |lib| 
  next true if lib.end_with?('.a', '.dylib')
  next true if lib.start_with?('lib')
  next true if lib =~ /^(lib)|([^a-zA-Z\d])/
  false
end
@squarefrog

squarefrog Apr 4, 2014

Contributor

This logic is covered by the regex. Anything that starts lib will be covered by ^(lib). While not explicit, the [a-zA-Z\d] picks up any strings with a file extension.

If you want the old behaviour for documentation purposes, then thats fine, but in terms of a matcher it's possibly redundant.

Owner

fabiopelosin commented Apr 4, 2014

This logic is covered by the regex. Anything that starts lib will be covered by ^(lib). While not explicit, the [a-zA-Z\d] picks up any strings with a file extension.

Sorry, I missed this point... so for me the code is fine as it is (without the duplication). The important is that the different tests cases are preserved to clarify the intent of the code and that the documentation at the top of each method explicits mentions the goals.

lib/cocoapods-core/specification/linter.rb
@@ -367,10 +367,11 @@ def _validate_compiler_flags(flags)
# @params frameworks [Array<String>]
# The frameworks to be validated
#
- # @return [Boolean] true if a framework ends in `.framework`
+ # @return [Boolean] true if a framework contains any
+ # non-alphanumeric character
@fabiopelosin

fabiopelosin Apr 4, 2014

Owner

I would prefer to be explicit: ... or includes an extension.

@squarefrog

squarefrog Apr 4, 2014

Contributor

@irrationalfab I agree that is much clearer. I'll add that now.

I did not remove any of the tests which test the old behaviour, as they exercise this new code as well. It's better to have too many tests than not enough!

Member

Kapin commented Apr 5, 2014

Looks good to me if @irrationalfab agrees 😸

@fabiopelosin fabiopelosin merged commit 8fd22cd into CocoaPods:master Apr 5, 2014

Owner

fabiopelosin commented Apr 5, 2014

Ace thanks for the patch!

Contributor

squarefrog commented Apr 5, 2014

It was my pleasure! Hopefully I can contribute again soon! Thanks for your support.

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