Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add validation to frameworks to make sure they are only names #41

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Member

Kapin commented Nov 30, 2013

The tests are failing and I can't figure out why (something mock related maybe?) but I believe this fixes Cocoapods#1336.

In terms of logic I think I'm on the right track but I think another set of eyes might be useful.

@Kapin Kapin Add validation to frameworks to make sure they are specified by name …
…only

Tests are failing and I don't know why but I believe the logic makes
sense.
b3d16f0

Coverage Status

Coverage decreased (-0.0%) when pulling b3d16f0 on Kapin:framework-linter-fix into 4ed00ac on CocoaPods:master.

Owner

alloy commented Dec 2, 2013

The logic definitely looks good to me. Have you been able to verify that the validation methods are being called?

Owner

alloy commented Dec 2, 2013

Since the frameworks and weak_frameworks attributes are shortcuts for other attributes, stubbing them did not lead to the desired behaviour. In these tests, stubbing is only needed if simply assigning the value doesn’t work. I’ve fixed it and pushed it here: https://github.com/CocoaPods/Core/tree/Kapin-framework-linter-fix.

PS: Should the libraries attribute not have the same type of linting? i.e. the name should be ‘foo’, not ‘libFoo.a’.

PPS: It’s probably easier if you push topic branches to our org repo instead of your own, this way it’s easier for us to push addition commits and have them show up in the PR.

Owner

fabiopelosin commented Dec 2, 2013

PS: Should the libraries attribute not have the same type of linting? i.e. the name should be ‘foo’, not ‘libFoo.a’.

👍

PPS: It’s probably easier if you push topic branches to our org repo instead of your own, this way it’s easier for us to push addition commits and have them show up in the PR.

👍

Member

Kapin commented Dec 2, 2013

@alloy I'll follow that PPS for now on. For the PS see #44. I'll open a separate PR for that too, but it's easier to keep track on things that need to be done in that issue 😄

Owner

fabiopelosin commented Dec 2, 2013

Ace can we merge the branch created by @alloy and close this pull request?

Member

Kapin commented Dec 2, 2013

@irrationalfab I want to add some documentation comments in there and make the error messages a tad more specific. I'll put those changes in and merge it when I get home from work.

Owner

fabiopelosin commented Dec 2, 2013

Ace!

Owner

alloy commented Dec 2, 2013

👍

Member

Kapin commented Dec 3, 2013

Closing to merge the branch instead.

@Kapin Kapin closed this Dec 3, 2013

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