Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages for podspec dependencies #474

Merged
merged 7 commits into from
Dec 19, 2018
Merged

Improve error messages for podspec dependencies #474

merged 7 commits into from
Dec 19, 2018

Conversation

Lutzifer
Copy link
Contributor

Improves the error message ’Unsupported version requirements' if it is caused by specifying :git or :path in the podspec file.

raise Informative, 'Unsupported version requirements'
unless version_requirements.is_a?(Hash)
version_requirements.each do |requirement|
raise Informative, "Podspecs can only use remote pods as dependencies. :path is not supported" if requirement[:path] != nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer if/elseif here

@dnkoutso
Copy link
Contributor

Needs tests for each path and a changelog entry

@orta
Copy link
Member

orta commented Nov 30, 2018

Nice idea 👍

@Lutzifer
Copy link
Contributor Author

Lutzifer commented Dec 3, 2018

@dnkoutso @orta
Thanks for the input. I think I solved the issues, could you please have another look?

if !requirement[:path].nil?
raise Informative, 'Podspecs can only use remote pods as dependencies. :path is not supported'
elsif !requirement[:git].nil?
raise Informative, 'Podspecs can only use remote pods as dependencies. :git is not supported'
Copy link
Member

@amorde amorde Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message isn't technically correct - you can use :git dependencies, but they cannot be specified by the podspec itself (ex. if the Podfile contains pod 'MyPod', :git => '...', a podspec with the dependency 'MyPod' will be fulfilled by that installation)

What about the following:

Podspecs cannot specify the source of dependencies. The :git option is not supported

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After rereading this for a couple of times, I think In understand what you mean.

Will change the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the same with path?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup :path behaves the same as far as I'm aware

CHANGELOG.md Outdated
@@ -4,6 +4,10 @@

##### Enhancements

* Better error messages, if unallowed version requirement is specified in Podspec.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add two extra spaces after . for formatting

@amorde
Copy link
Member

amorde commented Dec 5, 2018

LGTM once lint warnings are addressed

@dnkoutso dnkoutso added this to the 1.7 milestone Dec 14, 2018
version_requirements.each do |requirement|
if requirement.is_a?(Hash)
if !requirement[:path].nil?
raise Informative, 'Podspecs cannot specify the source of dependencies. The :path option is not supported.'\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit can you wrap :path with backticks ``? Same for :git below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@dnkoutso
Copy link
Contributor

ping @Lutzifer needs a rebase and a comment addressed

@Lutzifer
Copy link
Contributor Author

implemented the nit and did the rebase

@amorde amorde merged commit 9c31800 into CocoaPods:master Dec 19, 2018
@amorde
Copy link
Member

amorde commented Dec 19, 2018

Thanks @Lutzifer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants