Issue 1460 Subspec dependency checking should no longer produce false-po... #29

Merged
merged 2 commits into from Oct 10, 2013

Projects

None yet

6 participants

@nmccann
Contributor
nmccann commented Oct 10, 2013

Issue 1460 Subspec dependency checking should no longer produce false-positives

Previously, when defining dependencies in a pod spec, an error would be thrown if
the parent spec's name includes the dependency name (because it thinks a cyclic
dependency is created). For example, a pod spec named 'SpecTest' would incorrectly
fail if one of it's sub specs had a dependency on a pod named 'Spec'.

The new implementation looks for an exact match between a dependency's name and
all possible parents of the current subspec. For example if the name of a subspec's parent was
"Spec/Subspec/SubSubSpec", we would prevent the subspec from having a dependency on any
of the following:

  • Spec
  • Spec/Subspec
  • Spec/Subspec/SubSubSpec

This requires breaking the name into it's components and then recomposing it one by one, which
results in some admittedly messy code, Ruby isn't my primary language so I'm likely missing something
that would condense some of this code.

Since we are now looking for an exact match, there shouldn't be any false positives arising from
having a dependency with a similar name as one of the parent specs.

Note: I was unsure of where to put unit tests but if someone would like to describe the process to me
I can try to add some.

This is more or less my first foray into contributing to an open-source library so please be gentle :)

@nmccann nmccann Issue 1460 Subspec dependency checking should no longer produce false…
…-positives

Previously, when defining dependencies in a pod spec, an error would be thrown if
the parent spec's name includes the dependency name (because it thinks a cyclic
dependency is created). For example, a pod spec named 'SpecTest' would incorrectly
fail if one of it's sub specs had a dependency on a pod named 'Spec'.
58dda8e
@coveralls

Coverage Status

Coverage decreased (-0.19%) when pulling 58dda8e on nmccann:feature/subspec_dependency_fix into 95a4eda on CocoaPods:master.

@orta
Member
orta commented Oct 10, 2013

🍻

@fabiopelosin
Member

I was unsure of where to put unit tests but if someone would like to describe the process to me
I can try to add some.

The relevant specs are here.

To run the specs the process is the following:

$ [sudo] gem install bundler
$ bundle install
$ bundle exec rake spec

To run the spec of a file when it is saved you can do

$ rake spec:kick

This is more or less my first foray into contributing to an open-source library so please be gentle :)

This is fantastic work! You should be proud 😄. Btw, don't forget to add a not about this to the changelog in CocoaPods/CocoaPods crediting yourself (just a pull request is fine).

@sguillope

I'm proud of you @nmccann :)

@nmccann nmccann Issue 1460 Added unit tests for fix
Added an additional unit test to handle the case where a sub-sub-spec
is dependant upon a parent. Also added a unit test for the case where
a sub-spec is dependant on a pod whose name closely matches one of it's
parents names.
610651a
@nmccann
Contributor
nmccann commented Oct 10, 2013

Thanks guys :) I've added unit tests for an additional failing case, as well as a successful case. Let me know if there are some cases I haven't thought of.

I'll update the change log momentarily

@coveralls

Coverage Status

Coverage decreased (-0.02%) when pulling 610651a on nmccann:feature/subspec_dependency_fix into 95a4eda on CocoaPods:master.

@nmccann nmccann referenced this pull request in CocoaPods/CocoaPods Oct 10, 2013
Merged

[Changelog] #1468

@fabiopelosin
Member

Looks great! Thanks for the contribution!

@fabiopelosin fabiopelosin merged commit 97632f1 into CocoaPods:master Oct 10, 2013

1 check passed

default The Travis CI build passed
Details
@nmccann nmccann deleted the nmccann:feature/subspec_dependency_fix branch Oct 10, 2013
@alloy
Member
alloy commented Oct 10, 2013

@nmccann This is awesome, thanks so much and please keep it up! With this attitude you will likely have a great open-source future :)

@nmccann
Contributor
nmccann commented Oct 10, 2013

Thanks guys!

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