refactor CheckoutState #3666
refactor CheckoutState #3666
Conversation
tomerd
requested review from
abertelrud,
friedbunny and
neonichu
as code owners
tomerd
force-pushed
the
refactor/checkout-state
branch
2 times, most recently
from
a5856f2 to
bcd15fb
Compare
| return try manifest.dependencyConstraints(productFilter: productFilter) | ||
| } | ||
|
|
||
| func getDependencies(at revision: String, productFilter: ProductFilter) throws -> [PackageContainerConstraint] { | ||
| func getDependencies(at revisionString: String, productFilter: ProductFilter) throws -> [PackageContainerConstraint] { |
There was a problem hiding this comment.
please double check logic still looks right
| @@ -121,23 +122,25 @@ private struct LocalPackageContainer: PackageContainer { | |||
|
|
|||
| func getDependencies(at version: Version, productFilter: ProductFilter) throws -> [PackageContainerConstraint] { | |||
| // Because of the implementation of `reversedVersions`, we should only get the exact same version. | |||
| precondition(dependency?.checkoutState?.version == version) | |||
There was a problem hiding this comment.
please double check logic still looks right
| @@ -95,7 +96,7 @@ private struct LocalPackageContainer: PackageContainer { | |||
| let currentToolsVersion: ToolsVersion | |||
|
|
|||
| func versionsAscending() throws -> [Version] { | |||
| if let version = dependency?.state.checkout?.version { | |||
| if case .version(let version, revision: _) = dependency?.state.checkout { | |||
There was a problem hiding this comment.
please double check logic still looks right
| revision = pin.state.revision | ||
| if branch != nil, !updateBranches { | ||
| if case .branch(branch, let pinRevision) = pinsStore.pins.first(where: { $0.packageRef == packageRef })?.state { | ||
| revision = pinRevision |
There was a problem hiding this comment.
please double check logic still looks right
motivation: when working on configuration, CheckoutState data model stood out as one that can be improved changes: * transition CheckoutState from struct with optionals to enum representing the finite states * adust call sites and tests
tomerd
force-pushed
the
refactor/checkout-state
branch
from
bcd15fb to
19e23a0
Compare
|
@swift-ci please smoke test |
tomerd
added
the
ready
| } | ||
| case revision(Revision) | ||
| case version(Version, revision: Revision) | ||
| case branch(String, revision: Revision) |
There was a problem hiding this comment.
Should we name the first parameter, too?
There was a problem hiding this comment.
I started with naming them but it was a bit awkward in the call site e.g. let checkoutState: CheckoutState = .branch(branch: "main", revision: ...) vs let checkoutState: CheckoutState = .branch("main", revision: ...)
There was a problem hiding this comment.
Right, the first parameter is often unnamed when the enum case makes it obvious, and I agree that that applies here. So I think it should be unnamed.
There was a problem hiding this comment.
ended up adding name: to the branch variant
| } | ||
| case revision(Revision) | ||
| case version(Version, revision: Revision) | ||
| case branch(String, revision: Revision) |
There was a problem hiding this comment.
Right, the first parameter is often unnamed when the enum case makes it obvious, and I agree that that applies here. So I think it should be unnamed.
| @@ -95,7 +96,7 @@ private struct LocalPackageContainer: PackageContainer { | |||
| let currentToolsVersion: ToolsVersion | |||
|
|
|||
| func versionsAscending() throws -> [Version] { | |||
| if let version = dependency?.state.checkout?.version { | |||
| if case .version(let version, revision: _) = dependency?.state.checkout { | |||
| @@ -121,23 +122,25 @@ private struct LocalPackageContainer: PackageContainer { | |||
|
|
|||
| func getDependencies(at version: Version, productFilter: ProductFilter) throws -> [PackageContainerConstraint] { | |||
| // Because of the implementation of `reversedVersions`, we should only get the exact same version. | |||
| precondition(dependency?.checkoutState?.version == version) | |||
|
@swift-ci please smoke test |
motivation: when working on configuration, CheckoutState data model stood out as one that can be improved to be safer
changes: