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

[#212] Check for deployment target #241

Merged
merged 7 commits into from
May 4, 2012
Merged

Conversation

fabiopelosin
Copy link
Member

Going throughout pull request because I'm not sure of some points.

end
@platform = Platform.new(name, options)
@platform = Platform.new(name)
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any reason why @platform should receive the options in a specification. The options are currently used to pass the deployment target, Which is a Gem::Version instead in a specification the deployment target is a Gem::Requirement. This allows to specify deployment targets as:

  s.platform = :ios, '>= 5.0'
  s.platform = :ios, '~> 5.0'

Or something like (for discontinued pods):

  s.platform = :ios, ['>= 4.0', '< 6.0']

On the other using the Gem:Version would work assuming that the requirement is always >=.

@alloy
Copy link
Member

alloy commented Apr 30, 2012

I’ll check this out tomorrow.

@fabiopelosin
Copy link
Member Author

I'm starting to think that just using the Gem::Version might be the best approach. Xcode treats a deployment target as a >= requirement so it should suffice for pods as well. There are some cases where it does make sense to have more granular requirements (like some libraries that are a shim back-porting new features) however those cases are better left to the programmer.

@alloy
Copy link
Member

alloy commented May 3, 2012

Oops sorry, I totally forgot to check it. We had queensday on monday, I shouldn't respond to anything on such days :)

I'm starting to think that just using the Gem::Version might be the best approach. Xcode treats a deployment target as a >= requirement so it should suffice for pods as well.

Good point, let's just go with that for now.

* master:
  [Installer] Don't generate documenation if already installed
  [Command::Spec::Linter] Fix - Skip documentation generation
  [Config#doc] Renamed to generate_docs
  [Command::Spec::Linter] Bug fix.
  [# 242 Command::Spec::Linter] Refactoring
  [#246 | Resolver] Validate plaforms against each target.
  [Resolver] Check target plafroms instead of podfile.

Conflicts:
	lib/cocoapods/resolver.rb
- Allow to specify the deployment target after initialization
@fabiopelosin
Copy link
Member Author

Oops sorry, I totally forgot to check it. We had queensday on monday, I shouldn't respond to anything on such days :)

Nothing to be sorry about :-) Queensday looks like a really nice event!

@@ -8,22 +8,29 @@ def self.osx
new :osx
end

attr_reader :options
attr_reader :options, :deployment_target
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that the options attribute could be removed because it is not currently used.

@alloy
Copy link
Member

alloy commented May 4, 2012

Looks all good to me. Were there any specific questions you still wanted answered that I might have missed?

Btw, regarding setting up the feeds server, I will try to get to that tomorrow, it’s been a hectic week :)

@fabiopelosin
Copy link
Member Author

Looks all good to me. Were there any specific questions you still wanted answered that I might have missed?

Cool, I'm merging.

Btw, regarding setting up the feeds server, I will try to get to that tomorrow

Great :-)

it’s been a hectic week :)

Indeed...

fabiopelosin added a commit that referenced this pull request May 4, 2012
@fabiopelosin fabiopelosin merged commit fdcbd63 into master May 4, 2012
jzapater pushed a commit to jzapater/CocoaPods that referenced this pull request Sep 17, 2013
fabiopelosin added a commit that referenced this pull request Oct 25, 2014
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.

2 participants