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.

Sign up for GitHub

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

Jump to bottom

[Podspec] Add swift_version DSL #417

Merged
merged 1 commit into from
Dec 1, 2017
Merged

[Podspec] Add swift_version DSL #417

merged 1 commit into from
Dec 1, 2017

Conversation

endocrimes
Copy link
Member

dnkoutso
dnkoutso reviewed Oct 14, 2017
View reviewed changes
lib/cocoapods-core/specification/root_attribute_accessors.rb Outdated
# specification.
#
def swift_versions
@swift_versions ||= if versions = attributes_hash['swift_versions']
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the if versions = attributes_hash['swift_versions']

Does it also need to be set in the cocoapods_versions method or does that have a default value always set?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Requirement initializer covers the case of nil or empty string.

Copy link
Contributor

@dnkoutso dnkoutso Oct 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

but i guess we would prefer a nil value than a Requirement value with <Pod::Version version=0>?

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 prefer a nil here for the objective-c pod case

Copy link
Member

Choose a reason for hiding this comment

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

nil here is the right call

dnkoutso
dnkoutso reviewed Oct 14, 2017
View reviewed changes
lib/cocoapods-core/specification/dsl.rb

# @!method swift_version=(version)
#
# The version of Swift that the specification supports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably expand slightly more for the end user in which we document how this is used as a "best effort" value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, planning on following up with more info and docs

Copy link
Contributor

Choose a reason for hiding this comment

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

No longer need to do this, this is just a string.

@dnkoutso
Copy link
Contributor

👍

@orta
Copy link
Member

orta commented Oct 15, 2017

Feels like good foundations 👍

dnkoutso
dnkoutso reviewed Oct 16, 2017
View reviewed changes
lib/cocoapods-core/specification/dsl.rb Outdated
:container => Array,
:singularize => true,
:multi_platform => false,
:root_only => true
Copy link
Contributor

@dnkoutso dnkoutso Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I think root_attribute by default sets root_only property to true.

Copy link
Member

Choose a reason for hiding this comment

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

yup

@dnkoutso dnkoutso added this to the 1.4 milestone Oct 16, 2017
@siggb
Copy link

siggb commented Oct 18, 2017 •
edited
Loading

@dantoml Hello and thanks for improvements.

I've noticed same problem with spec dependencies: Cocoapods overrides SWIFT_VERSION on a Building with xcodebuild. step. Both spec.pod_target_xcconfig = { 'SWIFT_VERSION' => '4.0' } and .swift-version doesn't help.

As a result of pod spec lint --verbose I receive an error: The “Swift Language Version” (SWIFT_VERSION) build setting must be set to a supported value for targets which use Swift. This setting can be set in the build settings editor..

@endocrimes endocrimes mentioned this pull request Oct 18, 2017
Closed
@dnkoutso
Copy link
Contributor

@dantoml want me to rebase/merge?

segiddins
segiddins approved these changes Oct 18, 2017
View reviewed changes
lib/cocoapods-core/specification/root_attribute_accessors.rb Outdated
# specification.
#
def swift_versions
@swift_versions ||= if versions = attributes_hash['swift_versions']
Copy link
Member

Choose a reason for hiding this comment

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

nil here is the right call

lib/cocoapods-core/specification/dsl.rb Outdated
:container => Array,
:singularize => true,
:multi_platform => false,
:root_only => true
Copy link
Member

Choose a reason for hiding this comment

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

yup

spec/specification/root_attribute_accessors_spec.rb Outdated

it 'returns the requirement when only one is specified' do
@spec.swift_versions = '= 3.0'
@spec.swift_versions.should == Requirement.new('= 3.0')
Copy link
Member

Choose a reason for hiding this comment

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

nit: also test nil

@endocrimes
Copy link
Member Author

@dnkoutso waiting till I have the main pr ready too. I don’t like merging core changes till an integration pr is at least in review

@dnkoutso
Copy link
Contributor

ah! ok cool sorry didn't know :)

@orta orta mentioned this pull request Nov 8, 2017
Closed
@dnkoutso dnkoutso force-pushed the dani_swift_versions branch 2 times, most recently from 48264c1 to 135535f Compare December 1, 2017 18:21
@dnkoutso
Copy link
Contributor

dnkoutso commented Dec 1, 2017

Updated per our new design decision to scale down and not support ranges. Now swift_version is a simple string that is wrapped by a Version.

This will always be used if the pod author had set it, otherwise we fallback to what we do today.

PR for https://github.com/CocoaPods/CocoaPods coming up.

@dnkoutso dnkoutso force-pushed the dani_swift_versions branch 2 times, most recently from de995e6 to 51c466e Compare December 1, 2017 18:25
@dnkoutso
Copy link
Contributor

dnkoutso commented Dec 1, 2017

Any objections from merging this?

@dnkoutso dnkoutso force-pushed the dani_swift_versions branch from 51c466e to 7ad59a0 Compare December 1, 2017 18:47
@dnkoutso dnkoutso changed the title [Podspec] Add swift_versions attribute [Podspec] Add swift_version DSL Dec 1, 2017
@dnkoutso dnkoutso force-pushed the dani_swift_versions branch from 7ad59a0 to 839ffce Compare December 1, 2017 21:35
@dnkoutso
Copy link
Contributor

dnkoutso commented Dec 1, 2017

Going for it!

@dnkoutso dnkoutso merged commit 4943349 into master Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnkoutso dnkoutso dnkoutso approved these changes

@segiddins segiddins segiddins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.
5 participants
@endocrimes @dnkoutso @orta @siggb @segiddins