Enhance pod lib lint to support multiple Spec files in the folder #1635

Merged
merged 2 commits into from Dec 4, 2013

Conversation

Projects
None yet
3 participants
Contributor

olarivain commented Dec 1, 2013

This PR updates the pod lib lint command to support multiple spec files in the same folder, it currently raises if more than one pod spec is present, and doesn't take a command line argument like pod spec lint does.

This is quite useful when one forks a pod. The current behavior of requiring a single pod spec file in the current folder forces forked pods to remove (or entirely replace) the original pod file, which can create a big mess when sending PR to the original project.
The other option is to skip pod lib lint on builds, but you're the opening yourself to releasing completely broken pods.

Supporting multiple pod specs makes it possible to copy the original pod spec, rename the pod and push to a different Pod repo.
It's typically what we do when we need to fix something in third party pods, we append -OpenTable to the pod name and push it to our own repo.
It also makes it quite easy to revert back to the original pod, by simply dropping the suffix in the Podfile's dependencies.

Anyway, I've been careful to add some tests this time :)

Let me know if I should change anything

/kra

Owner

alloy commented Dec 2, 2013

While the change itself looks well done, I’m on the fence about merging it. The reason is that when you try to install a pod directly from its source repo, only 1 pod spec is assumed to be in the root of the repo. Now we could add more options to the Podfile DSL to select the spec, but the case needs to be really convincing to add options there.

In that context, can you explain how you work with your forked repos? And why it’s a problem to have changes to a spec when you’re going to send a PR, surely that means these changes need to be included in the PR?

Owner

fabiopelosin commented Dec 2, 2013

While the change itself looks well done, I’m on the fence about merging it. The reason is that when you try to install a pod directly from its source repo, only 1 pod spec is assumed to be in the root of the repo. Now we could add more options to the Podfile DSL to select the spec, but the case needs to be really convincing to add options there.

Not sure about how the logic handles this case however the assumption should be that there should be a podspec in the root of the repo named after the dependency. So unless I'm not considering something there should be no need for additional DSL directives.

However I don't understand the usage case as well.

Contributor

olarivain commented Dec 2, 2013

So the typical flow for us is to start by renaming the pod, so we can reference our forked pods without interfering with the upstream ones (version ranges can easily make this is a big headache if you don't do that, should upstream release after you release the fork). It's actually quite rare we have to touch anything else in the pod.

For instance, we have a patched version of AFNetworking that fixes the non escaping of [] characters on query strings. It took them quite a while to take the PR for that, and in the meantime we have to use the forked version. No big deal, I can just copy/paste the spec, rename it (AFNetworking-OT in our case) and pod push the spec to our own Podspec repo.
Then we can reference AFNetworking-OT in our Podfile/podspecs. When/if upstream accepts the PR, it's then quite simple to revert back to the original projet: just drop the -OT from the podspec/podfile.
Of course, our CI tracks do a pod lib lint before tagging and pushing, so with the current implementation I have no choice but to delete the original AFNetworking.podspec.
In short, I don't want to use Foo, I want to use my patched version of Foo. PR to Foo is on its way of course, but this can sometimes take a little while, and we have to ship in the meantime.

When you say "install a pod from its source repo", do you mean with :path option? I have to say, I haven't actually tested this. I'll give it a spin later today.

Owner

fabiopelosin commented Dec 2, 2013

Thanks I see your point now. I think @alloy's was referencing to the one of the options described here and here. However they don't appear to resolve the issue if you prefer to maintain a specs repo.

Owner

alloy commented Dec 2, 2013

My bad, you’re both completely right in that as long as you re-namespace the pod and its spec it will work just fine directly from a source repo. (By which I meant the repo where the actual lib code lives.)

Ok, so then I can definitely see the point more.

@irrationalfab Are you satisfied?

Owner

fabiopelosin commented Dec 3, 2013

Looks good to me. @olarivain can you add an entry to the changelog?

Contributor

olarivain commented Dec 3, 2013

Done.

Thanks guys!

fabiopelosin added a commit that referenced this pull request Dec 4, 2013

Merge pull request #1635 from opentable/multi-liblint
Enhance pod lib lint to support multiple Spec files in the folder

@fabiopelosin fabiopelosin merged commit f92bab0 into CocoaPods:master Dec 4, 2013

1 check passed

default The Travis CI build passed
Details
Owner

fabiopelosin commented Dec 4, 2013

Thank you for the patch!

Owner

alloy commented Dec 4, 2013

Indeed, thanks!

Contributor

olarivain commented Dec 5, 2013

Thank you for accepting it!

@olarivain olarivain deleted the opentable:multi-liblint branch Dec 5, 2013

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