Skip to content

Enforce use of throwing AbsolutePath initializer #353

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
Merged
merged 1 commit into from
Oct 19, 2022
Merged

Enforce use of throwing AbsolutePath initializer #353

merged 1 commit into from
Oct 19, 2022

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu self-assigned this Oct 14, 2022
@neonichu neonichu requested a review from tomerd as a code owner October 14, 2022 18:13
neonichu
neonichu commented Oct 14, 2022
View reviewed changes
Sources/TSCTestSupport/FileSystemExtensions.swift
@@ -72,3 +72,19 @@ extension FileSystem {
}
}
}

public extension AbsolutePath {
init(path: StaticString) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without an argument label, the String version of the initializer seemed to always get chosen, so I ended up just adding an argument label. These initializers are a convenience for tests which often deal with static paths.

@neonichu
Copy link
Contributor Author

@swift-ci please test

tomerd
tomerd reviewed Oct 14, 2022
View reviewed changes
tomerd
tomerd reviewed Oct 14, 2022
View reviewed changes
tomerd
tomerd approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

looks great. couple of small questions

@neonichu
Copy link
Contributor Author 

/home/build-user/swift-tools-support-core/Sources/TSCBasic/FileSystem.swift:358:33: error: extraneous argument label 'validating:' in call
        return try? AbsolutePath(validating: String(cString: fsr))
                                ^~~~~~~~~~~~~

Interesting, not sure why this didn't show up locally

@neonichu
Copy link
Contributor Author

@swift-ci please test

@neonichu neonichu force-pushed the AbsolutePath-initializer-enforce-throwing branch from 5b95470 to d1e5175 Compare October 17, 2022 18:49
@neonichu
Copy link
Contributor Author

@swift-ci please test

@neonichu neonichu force-pushed the AbsolutePath-initializer-enforce-throwing branch from d1e5175 to 785b81e Compare October 17, 2022 19:20
@neonichu
Copy link
Contributor Author

@swift-ci please test

@neonichu
Copy link
Contributor Author

Looks like at least on macOS we're reaching the swift-driver build now.

@neonichu neonichu force-pushed the AbsolutePath-initializer-enforce-throwing branch from 785b81e to dbdc714 Compare October 17, 2022 20:28
@neonichu
Copy link
Contributor Author

@swift-ci please test

@neonichu neonichu force-pushed the AbsolutePath-initializer-enforce-throwing branch 2 times, most recently from 5712700 to 7549b5d Compare October 18, 2022 06:48
@neonichu
Copy link
Contributor Author

Updated the approach slightly here, I am keeping AbsolutePath(validating:) and changed the relative path one to AbsolutePath(validating:relativeTo:). These changes, plus an extension that offers the non-throwing variants as deprecated, allow clients to move to the throwing variants piecemeal.

In a future PR, we can remove the deprecated initializers and validating argument labels.

@neonichu
Copy link
Contributor Author

@swift-ci please test

This was referenced Oct 18, 2022
Merged
Merged
neonichu added a commit to neonichu/sourcekit-lsp that referenced this pull request Oct 18, 2022
@neonichu
Adopt to path API changes
43c072d 
We are moving to a better model for TSC's path APIs in swiftlang/swift-tools-support-core#353. The previous API is still available (but deprecated) as much as possible, but since SourceKit-LSP was using `resolveSymlinks` (which is now throwing) quite a bit, there are some changes necessary.
@neonichu neonichu mentioned this pull request Oct 18, 2022
Merged
@neonichu neonichu force-pushed the AbsolutePath-initializer-enforce-throwing branch from 7549b5d to 83d3f52 Compare October 18, 2022 22:39
@neonichu
Copy link
Contributor Author

swiftlang/swift-package-manager#5797
swiftlang/swift-driver#1204
swiftlang/sourcekit-lsp#655
@swift-ci please test

neonichu added a commit to swiftlang/swift-driver that referenced this pull request Oct 19, 2022
@neonichu
Adopt to path API changes
89f817c 
We are moving to a better model for TSC's path APIs in swiftlang/swift-tools-support-core#353. The previous API is still available (but deprecated) as much as possible, but a small change is necessary here.
@neonichu neonichu merged commit 7c8bcf3 into swiftlang:main Oct 19, 2022
@neonichu neonichu deleted the AbsolutePath-initializer-enforce-throwing branch October 19, 2022 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomerd tomerd tomerd approved these changes

@aciidgh aciidgh Awaiting requested review from aciidgh

@abertelrud abertelrud Awaiting requested review from abertelrud

@friedbunny friedbunny Awaiting requested review from friedbunny

Assignees

@neonichu neonichu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@neonichu @tomerd