-
Notifications
You must be signed in to change notification settings - Fork 349
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
Use the podspec :subspec=>'xx'
to load dependencies from a subspec
#456
Conversation
I don't understand the issue this is trying to fix. How is this different from just specifying the subspec directly? Like this:
|
Maybe I lost some description. The project and targets are the development pods. If I use |
Ah, okay. So you're using this syntax https://guides.cocoapods.org/syntax/podfile.html#podspec to load the dependencies of the podspec. Is the issue that you want only the dependencies of the |
The |
Ok thanks, I think I understand now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A couple comments, and would like a least 1 other person to review
else | ||
get_hash_value('podspecs', []) << { :autodetect => true } | ||
options ||= {} | ||
unless options.keys.all? { |key| [:name, :path, :subspecs, :subspec].include?(key) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its worth adding only the plural :subspecs
option to keep the implementation simpler. If you only want 1 subspec, that could just be :subspecs => ['Subspec']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think about it. But I found that I often only use one subspec in a target, so the :subspec=>'xx'
is very simple and very common.
all_deps = all_specs.map { |s| s.dependencies(platform) }.flatten | ||
all_deps.reject { |dep| dep.root_name == spec.root.name } | ||
subspec_names = options[:subspecs] || options[:subspec] | ||
specs = if subspec_names.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this raise an error if the type is incorrect? Like if we receive :subspecs => [1234]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it strange that each parameter determines the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well they sort of have a type already, even if its implicit rather than explicit. Passing a Hash
or something else into :subspecs
doesn't really make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add subspec_names.map!(&:to_s)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seams that it will not raise an error if the subspecs => [1234]
.
subspec_names = options[:subspecs] || options[:subspec]
specs = if subspec_names.blank?
[spec]
else
subspec_names = [subspec_names] if subspec_names.is_a?(String)
subspec_names.map { |subspec_name| spec.subspec_by_name("#{spec.name}/#{subspec_name}") }
end
It will raise an error if :subspec=>123
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd rather not just convert everything to a String since that becomes an expected functionality of the API
It makes sense that if you are specifying a subspec name, it should be a String.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add the type checking in the store_podspec
method.
podfile.dependencies.map(&:name).should == %w(monkey AFNetworking SDWebImage) | ||
end | ||
|
||
it 'it can use use the dependencies of a podspec\'s subspec' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove the extra 'it'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂I just copy the description from last spec ( Line 402 ). Should I remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just the beginning so it's it 'can use the dependencies of a podspec\'s subspec' do
@amorde What description should I add into the changelog? |
Something like
|
943918f
to
19b27f1
Compare
@Whirlwind I think this is good to go! Could you squash the commits? |
+1 on squashing commits before we merge this |
19b27f1
to
a0675b9
Compare
Thanks @Whirlwind! |
Sometimes, I use the
podspec
in a Podfile to load dependencies from a.podspec
file. But I often just load one or some subspec(s) from the podspec file, not load all dependencies.eg: this is a Podspec:
this is a Podfile:
I need the target have different dependencies from different subspecs. This PR will add a option
subspec
(subspecs
) to thepodspec
DSL.