Skip to content

Commit d24083e

Browse files
milsemanKyle Macomber
andcommitted
Integrate API review
Fix bug in _modify and add better testing. Change lexicallyNormal to lexicallyNormalized() to match "ed" rule. Fix up some comments and remove some dead code Co-authored-by: Kyle Macomber <[email protected]>
1 parent 05e3d04 commit d24083e

File tree

7 files changed

+70
-210
lines changed

7 files changed

+70
-210
lines changed

Sources/System/FilePath/FilePathComponentView.swift

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ extension FilePath {
4343
public var components: ComponentView {
4444
__consuming get { ComponentView(self) }
4545
_modify {
46-
// RRC's empty init means that we cann't guarantee that the yielded
46+
// RRC's empty init means that we can't guarantee that the yielded
4747
// view will restore our root. So copy it out first.
4848
//
4949
// TODO(perf): Small-form root (especially on Unix). Have Root
@@ -54,13 +54,7 @@ extension FilePath {
5454
self = FilePath()
5555
defer {
5656
self = comp._path
57-
58-
if !rootStr.isEmpty {
59-
if let r = self.root {
60-
// Roots can be forgotten but never altered
61-
assert(r._slice.elementsEqual(rootStr))
62-
}
63-
// TODO: conditional on it having changed?
57+
if root?._slice.elementsEqual(rootStr) != true {
6458
self.root = Root(rootStr)
6559
}
6660
}
@@ -105,7 +99,6 @@ extension FilePath.ComponentView: BidirectionalCollection {
10599

106100
extension FilePath.ComponentView: RangeReplaceableCollection {
107101
public init() {
108-
// FIXME: but what about the root?
109102
self.init(FilePath())
110103
}
111104

Sources/System/FilePath/FilePathComponents.swift

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,6 @@ extension FilePath: _PlatformStringable {
199199

200200
}
201201

202-
// TODO(root): Re-think these initializers. We want them to be conv
203-
204202
extension FilePath.Component {
205203
// The index of the `.` denoting an extension
206204
internal func _extensionIndex() -> SystemString.Index? {

Sources/System/FilePath/FilePathParsing.swift

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,6 @@ extension FilePath {
158158
if writeIdx != relStart {
159159
let priorComponent = _parseComponent(priorTo: writeIdx)
160160

161-
// FIXME: it's not up until writeIdx because separator,
162-
// parseComponent should give the component...
163161
if !_isParentDirectory(priorComponent) {
164162
writeIdx = priorComponent.lowerBound
165163
readIdx = nextStart
@@ -182,8 +180,6 @@ extension FilePath {
182180

183181
assert(readIdx == _storage.endIndex && readIdx >= writeIdx)
184182
if readIdx != writeIdx {
185-
// TODO: Why was it everything after writeIdx?
186-
// storage.removeSubrange(storage.index(after: writeIdx)...)
187183
_storage.removeSubrange(writeIdx...)
188184
_removeTrailingSeparator()
189185
}
@@ -275,24 +271,6 @@ extension FilePath.ComponentView {
275271
internal var _relativeStart: SystemString.Index {
276272
_path._relativeStart
277273
}
278-
279-
internal func parseComponentStart(
280-
endingAt i: SystemString.Index
281-
) -> SystemString.Index {
282-
if i == _relativeStart, i != _path._storage.startIndex {
283-
return _path._storage.startIndex
284-
}
285-
var i = i
286-
if i != _path._storage.endIndex {
287-
assert(isSeparator(_path._storage[i]))
288-
i = _path._storage.index(before: i)
289-
}
290-
var slice = _path._storage[..<i]
291-
while let c = slice.last, !isSeparator(c) {
292-
slice.removeLast()
293-
}
294-
return slice.endIndex
295-
}
296274
}
297275

298276
extension SystemString {
@@ -370,7 +348,7 @@ extension FilePath {
370348
}
371349

372350
// Perform an append, inseting a separator if needed.
373-
// Note tha this will not check whether `content` is a root
351+
// Note that this will not check whether `content` is a root
374352
internal mutating func _append(unchecked content: Slice<SystemString>) {
375353
assert(FilePath(SystemString(content)).root == nil)
376354
if content.isEmpty { return }

Sources/System/FilePath/FilePathSyntax.swift

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -305,13 +305,13 @@ extension FilePath {
305305
/// replace the extension.
306306
///
307307
/// Examples:
308-
/// * `/tmp/foo.txt => txt`
309-
/// * `/Appliations/Foo.app/ => app`
310-
/// * `/Appliations/Foo.app/bar.txt => txt`
311-
/// * `/tmp/foo.tar.gz => gz`
312-
/// * `/tmp/.hidden => nil`
313-
/// * `/tmp/.hidden. => ""`
314-
/// * `/tmp/.. => nil`
308+
/// * `/tmp/foo.txt => txt`
309+
/// * `/Applications/Foo.app/ => app`
310+
/// * `/Applications/Foo.app/bar.txt => txt`
311+
/// * `/tmp/foo.tar.gz => gz`
312+
/// * `/tmp/.hidden => nil`
313+
/// * `/tmp/.hidden. => ""`
314+
/// * `/tmp/.. => nil`
315315
///
316316
/// Example:
317317
///
@@ -391,12 +391,10 @@ extension FilePath {
391391
/// Returns a copy of `self` in lexical-normal form, that is `.` and `..`
392392
/// components have been collapsed lexically (i.e. without following
393393
/// symlinks). See `lexicallyNormalize`
394-
public var lexicallyNormal: FilePath {
395-
__consuming get {
396-
var copy = self
397-
copy.lexicallyNormalize()
398-
return copy
399-
}
394+
public __consuming func lexicallyNormalized() -> FilePath {
395+
var copy = self
396+
copy.lexicallyNormalize()
397+
return copy
400398
}
401399

402400
/// Create a new `FilePath` by resolving `subpath` relative to `self`,
@@ -428,7 +426,7 @@ extension FilePath {
428426
public __consuming func lexicallyResolving(
429427
_ subpath: __owned FilePath
430428
) -> FilePath? {
431-
let subpath = subpath.removingRoot().lexicallyNormal
429+
let subpath = subpath.removingRoot().lexicallyNormalized()
432430
guard !subpath.isEmpty else { return self }
433431
guard subpath.components.first?.kind != .parentDirectory else {
434432
return nil

Tests/SystemTests/FilePathTests/FilePathComponentsTest.swift

Lines changed: 52 additions & 160 deletions
Original file line numberDiff line numberDiff line change
@@ -13,104 +13,6 @@ import SystemPackage
1313
@testable
1414
import SystemPackage
1515

16-
17-
// Helper to organize some ad-hoc testing
18-
//
19-
// TODO: Currently a class so we can overwrite file/line, but that can be
20-
// re-evaluated when we have source loc stacks.
21-
private final class AdHocComponentsTest: TestCase {
22-
// TODO (source-loc stack): Push fil/line from init onto stack
23-
24-
// Record the top-most file/line info (`expect` overwrites these values)
25-
//
26-
// TODO: When we have source loc stacks, push the location from the init,
27-
// and `expect` will be push/pops
28-
var file: StaticString
29-
var line: UInt
30-
31-
var path: FilePath
32-
var body: (AdHocComponentsTest) -> ()
33-
34-
init(
35-
_ path: FilePath,
36-
_ file: StaticString = #file,
37-
_ line: UInt = #line,
38-
_ body: @escaping (AdHocComponentsTest) -> ()
39-
) {
40-
self.file = file
41-
self.line = line
42-
self.path = path
43-
self.body = body
44-
}
45-
46-
func runAllTests() {
47-
body(self)
48-
}
49-
}
50-
51-
private func adhocComponentsTest(
52-
_ path: FilePath,
53-
_ file: StaticString = #file,
54-
_ line: UInt = #line,
55-
_ body: @escaping (AdHocComponentsTest) -> ()
56-
) {
57-
let test = AdHocComponentsTest(path, file, line, body)
58-
test.runAllTests()
59-
}
60-
61-
extension AdHocComponentsTest {
62-
// Temporarily re-bind file/line
63-
func withSourceLoc(
64-
_ newFile: StaticString,
65-
_ newLine: UInt,
66-
_ body: () -> ()
67-
) {
68-
let (origFile, origLine) = (self.file, self.line)
69-
(self.file, self.line) = (newFile, newLine)
70-
defer { (self.file, self.line) = (origFile, origLine) }
71-
body()
72-
}
73-
74-
// Customize error report by adding our path and components to output
75-
func failureMessage(_ reason: String?) -> String {
76-
"""
77-
78-
Fail
79-
path: \(path)
80-
components: \(Array(path.components))
81-
\(reason ?? "")
82-
"""
83-
}
84-
85-
func expect(
86-
_ expected: FilePath,
87-
file: StaticString = #file, line: UInt = #line
88-
) {
89-
90-
withSourceLoc(file, line) {
91-
expectEqual(expected, path, "expected: \(expected)")
92-
}
93-
}
94-
95-
func expectRelative(
96-
file: StaticString = #file, line: UInt = #line
97-
) {
98-
withSourceLoc(file, line) {
99-
expectTrue(path.isRelative, "expected relative")
100-
}
101-
}
102-
103-
func expectAbsolute(
104-
file: StaticString = #file, line: UInt = #line
105-
) {
106-
withSourceLoc(file, line) {
107-
expectTrue(path.isAbsolute, "expected absolute")
108-
}
109-
}
110-
111-
// TODO: Do we want to overload others like expectEqual[Sequence]?
112-
}
113-
11416
// @available(9999....)
11517
struct TestPathComponents: TestCase {
11618
var path: FilePath
@@ -152,69 +54,54 @@ struct TestPathComponents: TestCase {
15254
}
15355

15456
func testBidi() {
155-
156-
57+
expectEqualSequence(
58+
expectedComponents.reversed(), path.components.reversed(), "reversed()")
59+
expectEqualSequence(
60+
path.components, path.components.reversed().reversed(),
61+
"reversed().reversed()")
62+
for i in 0 ..< path.components.count {
63+
expectEqualSequence(
64+
expectedComponents.dropLast(i), path.components.dropLast(i), "dropLast")
65+
expectEqualSequence(
66+
expectedComponents.suffix(i), path.components.suffix(i), "suffix")
67+
}
15768
}
15869

15970
func testRRC() {
160-
// TODO: Convert tests into mutation tests
161-
// // What generalized tests can we do, given how initial "/" is special?
162-
// // E.g. absolute path inserted into itself can have only one root
163-
//
164-
// do {
165-
// var path = self.path
166-
// if path.isAbsolute {
167-
// path.components.removeFirst()
168-
// }
169-
// expectTrue(path.isRelative)
170-
//
171-
// let componentsArray = Array(path.components)
172-
// path.components.append(contentsOf: componentsArray)
173-
// expectEqualSequence(componentsArray + componentsArray, path.components)
174-
//
175-
// // TODO: Other generalized tests which work on relative paths
176-
// }
71+
// TODO: programmatic tests showing parity with Array<Component>
72+
}
73+
74+
func testModify() {
75+
if path.root == nil {
76+
let rootedPath = FilePath(root: "/", path.components)
77+
expectNotEqual(rootedPath, path)
78+
var pathCopy = path
79+
expectEqual(path, pathCopy)
80+
pathCopy.components = rootedPath.components
81+
expectNil(pathCopy.root, "components.set doesn't assign root")
82+
expectEqual(path, pathCopy)
83+
} else {
84+
let rootlessPath = FilePath(root: nil, path.components)
85+
var pathCopy = path
86+
expectEqual(path, pathCopy)
87+
pathCopy.components = rootlessPath.components
88+
expectNotNil(pathCopy.root, "components.set preserves root")
89+
expectEqual(path, pathCopy)
90+
}
17791
}
17892

17993
func runAllTests() {
18094
testComponents()
18195
testBidi()
18296
testRRC()
97+
testModify()
18398
}
18499
}
185100

186101
// TODO: Note that double-reversal will drop root if FilePath is constructed in between ...
187102

188103
// @available(macOS 10.16, iOS 14.0, watchOS 7.0, tvOS 14.0, *)
189104
final class FilePathComponentsTest: XCTestCase {
190-
191-
let testPaths: Array<TestPathComponents> = [
192-
TestPathComponents("", root: nil, []),
193-
TestPathComponents("/", root: "/", []),
194-
TestPathComponents("foo", root: nil, ["foo"]),
195-
TestPathComponents("foo/", root: nil, ["foo"]),
196-
TestPathComponents("/foo", root: "/", ["foo"]),
197-
TestPathComponents("foo/bar", root: nil, ["foo", "bar"]),
198-
TestPathComponents("foo/bar/", root: nil, ["foo", "bar"]),
199-
TestPathComponents("/foo/bar", root: "/", ["foo", "bar"]),
200-
TestPathComponents("///foo//", root: "/", ["foo"]),
201-
TestPathComponents("/foo///bar", root: "/", ["foo", "bar"]),
202-
TestPathComponents("foo/bar/", root: nil, ["foo", "bar"]),
203-
TestPathComponents("foo///bar/baz/", root: nil, ["foo", "bar", "baz"]),
204-
TestPathComponents("//foo///bar/baz/", root: "/", ["foo", "bar", "baz"]),
205-
TestPathComponents("./", root: nil, ["."]),
206-
TestPathComponents("./..", root: nil, [".", ".."]),
207-
TestPathComponents("/./..//", root: "/", [".", ".."]),
208-
]
209-
210-
// TODO: generalize to a driver protocol that will inherit from XCTest, expose allTestCases
211-
// based on an associated type, and provide the testCasees func, assuming XCTest supports
212-
// that.
213-
func testCases() {
214-
testPaths.forEach { $0.runAllTests() }
215-
}
216-
217-
// TODO: Convert these kinds of test cases into mutation API test cases.
218105
func testAdHocRRC() {
219106
var path: FilePath = "/usr/local/bin"
220107

@@ -343,21 +230,26 @@ final class FilePathComponentsTest: XCTestCase {
343230
}
344231
}
345232

346-
func testConcatenation() {
347-
// TODO: convert tests into mutation tests
348-
349-
// for lhsTest in testPaths {
350-
// let lhs = lhsTest.path
351-
// for rhsTest in testPaths {
352-
// let rhs = rhsTest.path
353-
// adhocComponentsTest(lhs + rhs) { concatpath in
354-
// // (lhs + rhs).components == (lhs.components + rhs.compontents)
355-
// concatpath.expectEqualSequence(lhs.components + rhs.components, concatpath.components)
356-
//
357-
// // TODO: More tests around non-normalized separators
358-
// }
359-
// }
360-
// }
233+
func testCases() {
234+
let testPaths: Array<TestPathComponents> = [
235+
TestPathComponents("", root: nil, []),
236+
TestPathComponents("/", root: "/", []),
237+
TestPathComponents("foo", root: nil, ["foo"]),
238+
TestPathComponents("foo/", root: nil, ["foo"]),
239+
TestPathComponents("/foo", root: "/", ["foo"]),
240+
TestPathComponents("foo/bar", root: nil, ["foo", "bar"]),
241+
TestPathComponents("foo/bar/", root: nil, ["foo", "bar"]),
242+
TestPathComponents("/foo/bar", root: "/", ["foo", "bar"]),
243+
TestPathComponents("///foo//", root: "/", ["foo"]),
244+
TestPathComponents("/foo///bar", root: "/", ["foo", "bar"]),
245+
TestPathComponents("foo/bar/", root: nil, ["foo", "bar"]),
246+
TestPathComponents("foo///bar/baz/", root: nil, ["foo", "bar", "baz"]),
247+
TestPathComponents("//foo///bar/baz/", root: "/", ["foo", "bar", "baz"]),
248+
TestPathComponents("./", root: nil, ["."]),
249+
TestPathComponents("./..", root: nil, [".", ".."]),
250+
TestPathComponents("/./..//", root: "/", [".", ".."]),
251+
]
252+
testPaths.forEach { $0.runAllTests() }
361253
}
362254

363255
func testSeparatorNormalization() {

Tests/SystemTests/FilePathTests/FilePathExtras.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ extension FilePath {
5050
guard !other.isEmpty else { return true }
5151
guard !isEmpty else { return false }
5252

53-
let (selfLex, otherLex) = (self.lexicallyNormal, other.lexicallyNormal)
53+
let (selfLex, otherLex) =
54+
(self.lexicallyNormalized(), other.lexicallyNormalized())
5455
if otherLex.isAbsolute { return selfLex.starts(with: otherLex) }
5556

5657
// FIXME: Windows semantics with relative roots?

0 commit comments

Comments
 (0)