Skip to content

Commit a5856f2

Browse files
tomerdtomerd
committed
refactor CheckoutState
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
1 parent 7756de5 commit a5856f2

File tree

9 files changed

+177
-100
lines changed

9 files changed

+177
-100
lines changed

‎Sources/PackageGraph/CheckoutState.swift

Lines changed: 73 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -15,50 +15,44 @@ import TSCUtility
1515
/// A checkout state represents the current state of a repository.
1616
///
1717
/// A state will always has a revision. It can also have a branch or a version but not both.
18-
public struct CheckoutState: Equatable, Hashable {
18+
public enum CheckoutState: Equatable, Hashable {
1919

20-
/// The revision of the checkout.
21-
public let revision: Revision
22-
23-
/// The version of the checkout, if known.
24-
public let version: Version?
25-
26-
/// The branch of the checkout, if known.
27-
public let branch: String?
28-
29-
/// Create a checkout state with given data. It is invalid to provide both version and branch.
30-
///
31-
/// This is deliberately fileprivate so CheckoutState is not initialized
32-
/// with both version and branch. All other initializers should delegate to it.
33-
fileprivate init(revision: Revision, version: Version?, branch: String?) {
34-
assert(version == nil || branch == nil, "Can't set both branch and version.")
35-
self.revision = revision
36-
self.version = version
37-
self.branch = branch
38-
}
39-
40-
/// Create a checkout state with given revision and branch.
41-
public init(revision: Revision, branch: String? = nil) {
42-
self.init(revision: revision, version: nil, branch: branch)
43-
}
20+
case revision(Revision)
21+
case version(Version, revision: Revision)
22+
case branch(String, revision: Revision)
4423

45-
/// Create a checkout state with given revision and version.
46-
public init(revision: Revision, version: Version) {
47-
self.init(revision: revision, version: version, branch: nil)
24+
/// The revision of the checkout.
25+
public var revision: Revision {
26+
get {
27+
switch self {
28+
case .revision(let revision):
29+
return revision
30+
case .version(_, let revision):
31+
return revision
32+
case .branch(_, let revision):
33+
return revision
34+
}
35+
}
4836
}
4937

5038
public var isBranchOrRevisionBased: Bool {
51-
return version == nil
39+
switch self {
40+
case .revision, .branch:
41+
return true
42+
case .version:
43+
return false
44+
}
5245
}
5346

5447
/// Returns requirement induced by this state.
5548
public var requirement: PackageRequirement {
56-
if let version = version {
49+
switch self {
50+
case .revision(let revision):
51+
return .revision(revision.identifier)
52+
case .version(let version, _):
5753
return .versionSet(.exact(version))
58-
} else if let branch = branch {
54+
case .branch(let branch, _):
5955
return .revision(branch)
60-
} else {
61-
return .revision(revision.identifier)
6256
}
6357
}
6458
}
@@ -67,27 +61,62 @@ public struct CheckoutState: Equatable, Hashable {
6761

6862
extension CheckoutState: CustomStringConvertible {
6963
public var description: String {
70-
return version?.description ?? branch ?? revision.identifier
64+
switch self {
65+
case .revision(let revision):
66+
return revision.identifier
67+
case .version(let version, _):
68+
return version.description
69+
case .branch(let branch, _):
70+
return branch
71+
}
7172
}
7273
}
7374

7475
// MARK: - JSON
7576

7677
extension CheckoutState: JSONMappable, JSONSerializable {
7778
public init(json: JSON) throws {
78-
self.init(
79-
revision: try json.get("revision"),
80-
version: json.get("version"),
81-
branch: json.get("branch")
82-
)
79+
let revision: Revision = try json.get("revision")
80+
let version: Version? = json.get("version")
81+
let branch: String? = json.get("branch")
82+
83+
switch (version, branch) {
84+
case (.none, .none):
85+
self = .revision(revision)
86+
case (.some(let version), .none):
87+
self = .version(version, revision: revision)
88+
case (.none, .some(let branch)):
89+
self = .branch(branch, revision: revision)
90+
case (.some(_), .some(_)):
91+
preconditionFailure("Can't set both branch and version.")
92+
}
8393
}
8494

8595
public func toJSON() -> JSON {
86-
return .init([
87-
"revision": revision.identifier,
88-
"version": version.toJSON(),
89-
"branch": branch.toJSON(),
90-
])
96+
let revision: Revision
97+
let version: Version?
98+
let branch: String?
99+
100+
switch self {
101+
case .revision(let _revision):
102+
revision = _revision
103+
version = nil
104+
branch = nil
105+
case .version(let _version, let _revision):
106+
revision = _revision
107+
version = _version
108+
branch = nil
109+
case .branch(let _branch, let _revision):
110+
revision = _revision
111+
version = nil
112+
branch = _branch
113+
}
114+
115+
return .init([
116+
"revision": revision.identifier,
117+
"version": version.toJSON(),
118+
"branch": branch.toJSON(),
119+
])
91120
}
92121
}
93122

‎Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,8 @@ public struct PubgrubDependencyResolver {
346346
// latest commit on that branch. Note that if this revision-based dependency is
347347
// already a commit, then its pin entry doesn't matter in practice.
348348
let revisionForDependencies: String
349-
if let pin = pinsMap[package.identity], pin.state.branch == revision {
350-
revisionForDependencies = pin.state.revision.identifier
349+
if case .branch(revision, let pinRevision) = pinsMap[package.identity]?.state {
350+
revisionForDependencies = pinRevision.identifier
351351

352352
// Mark the package as overridden with the pinned revision and record the branch as well.
353353
overriddenPackages[package] = (version: .revision(revisionForDependencies, branch: revision), products: constraint.products)
@@ -1054,7 +1054,13 @@ private final class PubGrubPackageContainer {
10541054

10551055
/// Returns the pinned version for this package, if any.
10561056
var pinnedVersion: Version? {
1057-
return self.pinsMap[self.underlying.package.identity]?.state.version
1057+
switch self.pinsMap[self.underlying.package.identity]?.state {
1058+
case .version(let version, _):
1059+
return version
1060+
default:
1061+
return .none
1062+
}
1063+
//return self.pinsMap[self.underlying.package.identity]?.state.version
10581064
}
10591065

10601066
/// Returns the numbers of versions that are satisfied by the given version requirement.

‎Sources/SPMTestSupport/MockWorkspace.swift

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -683,3 +683,31 @@ public final class MockWorkspaceDelegate: WorkspaceDelegate {
683683
}
684684
}
685685
}
686+
687+
extension CheckoutState {
688+
public var version: Version? {
689+
get {
690+
switch self {
691+
case .revision:
692+
return .none
693+
case .version(let version, _):
694+
return version
695+
case .branch:
696+
return .none
697+
}
698+
}
699+
}
700+
701+
public var branch: String? {
702+
get {
703+
switch self {
704+
case .revision:
705+
return .none
706+
case .version:
707+
return .none
708+
case .branch(let branch, _):
709+
return branch
710+
}
711+
}
712+
}
713+
}

‎Sources/Workspace/ResolverPrecomputationProvider.swift

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,13 @@
88
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
99
*/
1010

11+
import Basics
1112
import Dispatch
12-
import PackageModel
1313
import PackageGraph
14+
import PackageModel
15+
import SourceControl
1416
import TSCBasic
1517
import TSCUtility
16-
import SourceControl
1718

1819
/// Enumeration of the different errors that can arise from the `ResolverPrecomputationProvider` provider.
1920
enum ResolverPrecomputationError: Error {
@@ -95,7 +96,7 @@ private struct LocalPackageContainer: PackageContainer {
9596
let currentToolsVersion: ToolsVersion
9697

9798
func versionsAscending() throws -> [Version] {
98-
if let version = dependency?.state.checkout?.version {
99+
if case .version(let version, revision: _) = dependency?.state.checkout {
99100
return [version]
100101
} else {
101102
return []
@@ -121,23 +122,25 @@ private struct LocalPackageContainer: PackageContainer {
121122

122123
func getDependencies(at version: Version, productFilter: ProductFilter) throws -> [PackageContainerConstraint] {
123124
// Because of the implementation of `reversedVersions`, we should only get the exact same version.
124-
precondition(dependency?.checkoutState?.version == version)
125+
guard case .version(version, revision: _) = dependency?.checkoutState else {
126+
throw InternalError("expected version, but checkout state was \(String(describing: dependency?.checkoutState))")
127+
}
125128
return try manifest.dependencyConstraints(productFilter: productFilter)
126129
}
127130

128131
func getDependencies(at revision: String, productFilter: ProductFilter) throws -> [PackageContainerConstraint] {
129132
// Return the dependencies if the checkout state matches the revision.
130-
if let checkoutState = dependency?.checkoutState,
131-
checkoutState.version == nil,
132-
checkoutState.revision.identifier == revision {
133+
switch dependency?.checkoutState {
134+
case .branch(_, revision: let _revision) where _revision.identifier == revision,
135+
.revision(let _revision) where _revision.identifier == revision:
133136
return try manifest.dependencyConstraints(productFilter: productFilter)
137+
default:
138+
throw ResolverPrecomputationError.differentRequirement(
139+
package: self.package,
140+
state: self.dependency?.state,
141+
requirement: .revision(revision)
142+
)
134143
}
135-
136-
throw ResolverPrecomputationError.differentRequirement(
137-
package: self.package,
138-
state: self.dependency?.state,
139-
requirement: .revision(revision)
140-
)
141144
}
142145

143146
func getUnversionedDependencies(productFilter: ProductFilter) throws -> [PackageContainerConstraint] {

‎Sources/Workspace/Workspace.swift

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1455,10 +1455,15 @@ extension Workspace {
14551455
switch managedDependency.state {
14561456
case .checkout(let checkoutState):
14571457
packageKind = .remote
1458-
version = checkoutState.version
1458+
switch checkoutState {
1459+
case .version(let checkoutVersion, _):
1460+
version = checkoutVersion
1461+
default:
1462+
version = .none
1463+
}
14591464
case .edited, .local:
14601465
packageKind = .local
1461-
version = nil
1466+
version = .none
14621467
}
14631468

14641469
// Get the path of the package.
@@ -2355,19 +2360,22 @@ extension Workspace {
23552360

23562361
// If we have a branch and we shouldn't be updating the
23572362
// branches, use the revision from pin instead (if present).
2358-
if branch != nil {
2359-
if let pin = pinsStore.pins.first(where: { $0.packageRef == packageRef }),
2360-
!updateBranches,
2361-
pin.state.branch == branch {
2362-
revision = pin.state.revision
2363+
if branch != nil, !updateBranches {
2364+
if case .branch(branch, let pinRevision) = pinsStore.pins.first(where: { $0.packageRef == packageRef })?.state {
2365+
revision = pinRevision
23632366
}
23642367
}
23652368

23662369
// First check if we have this dependency.
23672370
if let currentDependency = currentDependency {
23682371
// If current state and new state are equal, we don't need
23692372
// to do anything.
2370-
let newState = CheckoutState(revision: revision, branch: branch)
2373+
let newState: CheckoutState
2374+
if let branch = branch {
2375+
newState = .branch(branch, revision: revision)
2376+
} else {
2377+
newState = .revision(revision)
2378+
}
23712379
if case .checkout(let checkoutState) = currentDependency.state, checkoutState == newState {
23722380
packageStateChanges[packageRef.location] = (packageRef, .unchanged)
23732381
} else {
@@ -2382,7 +2390,7 @@ extension Workspace {
23822390

23832391
case .version(let version):
23842392
if let currentDependency = currentDependency {
2385-
if case .checkout(let checkoutState) = currentDependency.state, checkoutState.version == version {
2393+
if case .checkout(let checkoutState) = currentDependency.state, case .version(let checkoutVersion, _) = checkoutState, checkoutVersion == version {
23862394
packageStateChanges[packageRef.location] = (packageRef, .unchanged)
23872395
} else {
23882396
let newState = PackageStateChange.State(requirement: .version(version), products: products)
@@ -2664,10 +2672,13 @@ extension Workspace {
26642672
throw InternalError("unable to get tag for \(package) \(version); available versions \(try container.versionsDescending())")
26652673
}
26662674
let revision = try container.getRevision(forTag: tag)
2667-
checkoutState = CheckoutState(revision: revision, version: version)
2675+
checkoutState = .version(version, revision: revision)
2676+
2677+
case .revision(let revision, .none):
2678+
checkoutState = .revision(revision)
26682679

2669-
case .revision(let revision, let branch):
2670-
checkoutState = CheckoutState(revision: revision, branch: branch)
2680+
case .revision(let revision, .some(let branch)):
2681+
checkoutState = .branch(branch, revision: revision)
26712682

26722683
case .unversioned:
26732684
state.dependencies.add(ManagedDependency.local(packageRef: package))

‎Tests/CommandsTests/PackageToolTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -723,7 +723,7 @@ final class PackageToolTests: XCTestCase {
723723
do {
724724
try execute("resolve", "bar", "--branch", "YOLO")
725725
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
726-
let state = CheckoutState(revision: yoloRevision, branch: "YOLO")
726+
let state = CheckoutState.branch("YOLO", revision: yoloRevision)
727727
let identity = PackageIdentity(path: barPath)
728728
XCTAssertEqual(pinsStore.pinsMap[identity]?.state, state)
729729
}
@@ -732,7 +732,7 @@ final class PackageToolTests: XCTestCase {
732732
do {
733733
try execute("resolve", "bar", "--revision", yoloRevision.identifier)
734734
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
735-
let state = CheckoutState(revision: yoloRevision)
735+
let state = CheckoutState.revision(yoloRevision)
736736
let identity = PackageIdentity(path: barPath)
737737
XCTAssertEqual(pinsStore.pinsMap[identity]?.state, state)
738738
}

‎Tests/PackageGraphTests/PubgrubTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1902,11 +1902,11 @@ final class PubGrubBacktrackTests: XCTestCase {
19021902
fileprivate extension CheckoutState {
19031903
/// Creates a checkout state with the given version and a mocked revision.
19041904
static func version(_ version: Version) -> CheckoutState {
1905-
CheckoutState(revision: Revision(identifier: "<fake-ident>"), version: version)
1905+
.version(version, revision: Revision(identifier: "<fake-ident>"))
19061906
}
19071907

19081908
static func branch(_ branch: String, revision: String) -> CheckoutState {
1909-
CheckoutState(revision: Revision(identifier: revision), branch: branch)
1909+
.branch(branch, revision: Revision(identifier: revision))
19101910
}
19111911
}
19121912

0 commit comments

Comments
 (0)