Skip to content

Make it impossible to create an invalid BuildSettingCondition#4131

Merged
tomerd merged 1 commit into
swiftlang:mainfrom
sjavora:disallowImpossibleBuildSettingCondition
Feb 15, 2022
Merged

Make it impossible to create an invalid BuildSettingCondition#4131
tomerd merged 1 commit into
swiftlang:mainfrom
sjavora:disallowImpossibleBuildSettingCondition

Conversation

@sjavora

@sjavora sjavora commented Feb 12, 2022

Copy link
Copy Markdown
Contributor

Instead of a single when method that takes two optional values, use multiple methods so that it is impossible for both to be nil.

tomerd
tomerd reviewed Feb 12, 2022
View reviewed changes
Comment thread Sources/PackageDescription/BuildSettings.swift Outdated
/// - Parameters:
/// - platforms: The applicable platforms for this build setting condition.
public static func when(platforms: [Platform]) -> BuildSettingCondition {
BuildSettingCondition(platforms: platforms, config: nil)

@tomerd tomerd Feb 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: use .none instead of nil to make it more semantically clear

tomerd
tomerd reviewed Feb 12, 2022
View reviewed changes
Comment thread Sources/PackageDescription/BuildSettings.swift Outdated
precondition(!(platforms == nil && configuration == nil))
return BuildSettingCondition(platforms: platforms, config: configuration)
public static func when(configuration: BuildConfiguration) -> BuildSettingCondition {
BuildSettingCondition(platforms: nil, config: configuration)

@tomerd tomerd Feb 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: use .none instead of nil to make it more semantically clear

@tomerd

tomerd commented Feb 12, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@tomerd

tomerd commented Feb 12, 2022

Copy link
Copy Markdown
Contributor

thanks @sjavora

@sjavora sjavora force-pushed the disallowImpossibleBuildSettingCondition branch from 23756a4 to fbe0d7a Compare February 12, 2022 20:09
@sjavora

sjavora commented Feb 12, 2022

Copy link
Copy Markdown
Contributor Author

@tomerd Updated to use .none 👍

In the meantime I also found https://github.com/apple/swift-package-manager/blob/2ea136b5703039b5a596c8a61d090e4dc300ee98/Sources/PackageDescription/PackageDescriptionSerialization.swift#L354

...which seems like it got copy/pasted as there is just one optional parameter. Should I remove that as well? TargetDependencyCondition is Encodable so I'm not 100% sure removing the optionality won't break something...

@tomerd

tomerd commented Feb 12, 2022

Copy link
Copy Markdown
Contributor

thanks @sjavora

PackageDescriptionSerialization is a bit more sensitive since it drives the manifest parsing. this does need to be improved, but maybe a topic for a separate PR

@tomerd

tomerd commented Feb 12, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Feb 12, 2022
@SDGGiesbrecht

SDGGiesbrecht commented Feb 12, 2022

Copy link
Copy Markdown
Contributor

This is a public, source‐breaking change to the PackageDescription module. (For example, .when(platforms: [.macOS], configuration: nil) will break.) Does the old method need to be retained with tools version constraints and/or deprecation notices?

@tomerd

tomerd commented Feb 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is a public, source‐breaking change to the PackageDescription module. (For example, .when(platforms: [.macOS], configuration: nil) will break.) Does the old method need to be retained with tools version constraints and/or deprecation notices?

good catch @SDGGiesbrecht did not notice this was in PackageDescription. @sjavora please lets restore the previous API but mark as deprecated, and add availability notation on the new APIs as well e.g..

@available(_PackageDescription, deprecated: 999.0)
public static func when(
         platforms: [Platform]? = nil,
         configuration: BuildConfiguration? = nil
     ) -> BuildSettingCondition

@available(_PackageDescription, introduced: 999.0)
public static func when(
         platforms: [Platform]
     ) -> BuildSettingCondition

we will update 999.0 to 5.7 when we add that version identifier

@sjavora sjavora force-pushed the disallowImpossibleBuildSettingCondition branch from fbe0d7a to a2bc957 Compare February 13, 2022 10:11
@sjavora
Make it impossible to create an invalid BuildSettingCondition.
60f0253 
Instead of a single method that takes two optional values, use multiple methods so that it is impossible for both to be `nil`.
@sjavora sjavora force-pushed the disallowImpossibleBuildSettingCondition branch from a2bc957 to 60f0253 Compare February 13, 2022 10:13
@sjavora

sjavora commented Feb 13, 2022

Copy link
Copy Markdown
Contributor Author

Oops! I didn't even think about that 🤦

Thank you for noticing - put back the original method and added @availability.

@tomerd

tomerd commented Feb 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

thanks @sjavora this looks good now imo. @SDGGiesbrecht wdyt?

one last ask, since this is user facing API change in the manifest could you please add an entry in the change log, e.g. #4137 (can be done in a separate PR if you prefer)

@SDGGiesbrecht

SDGGiesbrecht commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

👍 Looks good now.

@tomerd

tomerd commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@shahmishal

shahmishal commented Feb 14, 2022

Copy link
Copy Markdown
Member

@swift-ci please smoke test

4 similar comments
@tomerd

tomerd commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@tomerd

tomerd commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@tomerd

tomerd commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@tomerd

tomerd commented Feb 15, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@shahmishal

shahmishal commented Feb 15, 2022

Copy link
Copy Markdown
Member

@swift-ci please smoke test

1 similar comment
@tomerd

tomerd commented Feb 15, 2022

Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@tomerd tomerd merged commit 645fe16 into swiftlang:main Feb 15, 2022
tomerd added a commit that referenced this pull request Feb 15, 2022
@tomerd
Update CHANGELOG.md
a57768e 
motivation: keep change log up to date

changes: document changes from #4131
@tomerd tomerd mentioned this pull request Feb 15, 2022
Merged
@sjavora sjavora deleted the disallowImpossibleBuildSettingCondition branch February 15, 2022 08:11
tomerd added a commit that referenced this pull request Feb 15, 2022
@tomerd
Update CHANGELOG.md (#4141)
a06b331 
motivation: keep change log up to date

changes: document changes from #4131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abertelrud abertelrud Awaiting requested review from abertelrud
@neonichu neonichu Awaiting requested review from neonichu
@elsh elsh Awaiting requested review from elsh
1 more reviewer
@tomerd tomerd tomerd approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

add to change log ready Author believes the PR is ready to be merged & any feedback has been addressed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sjavora @tomerd @SDGGiesbrecht @shahmishal