Skip to content
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.

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

Support a relative path for the options to "link to source" for the --checkout-path CLI option #490 #929

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions Sources/SwiftDocC/SourceRepository/SourceRepository.swift
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a significant amount of whitespace-only changes in all these files which adds noise to the PR. Please remove all those.

Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,40 @@
*/

import Foundation
import ArgumentParser
Anthony-Eid marked this conversation as resolved.
Show resolved Hide resolved

/// A remote repository that hosts source code.
public struct SourceRepository {
/// The path at which the repository is cloned locally.
public var checkoutPath: String
Copy link
Contributor

Choose a reason for hiding this comment

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

If this was defined as a URL then the issue with absolute and relative paths would already be handled by the URL(fileURLWithPath:) initializer.


/// The base URL where the service hosts the repository's contents.
public var sourceServiceBaseURL: URL

/// A function that formats a line number to be included in a URL.
public var formatLineNumber: (Int) -> String

/// Creates a source code repository.
/// - Parameters:
/// - checkoutPath: The path at which the repository is checked out locally and from which its symbol graphs were generated.
/// - sourceServiceBaseURL: The base URL where the service hosts the repository's contents.
/// - formatLineNumber: A function that formats a line number to be included in a URL.
public init(
public init (
checkoutPath: String,
sourceServiceBaseURL: URL,
formatLineNumber: @escaping (Int) -> String
) {
self.checkoutPath = checkoutPath

// Get the absolute path of a file without the file:// prefix because this function used to only
// expect absolute paths from a user and didn't convert checkoutPath to a URL and back.
let absoluteCheckoutPath = URL(fileURLWithPath: checkoutPath).absoluteString
let startIndex = absoluteCheckoutPath.index(absoluteCheckoutPath.startIndex, offsetBy: 7)
self.checkoutPath = String(absoluteCheckoutPath[startIndex...])
Comment on lines +36 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never seen this pattern before and I can't find an input where it behaves differently than URL(fileURLWithPath: checkoutPath).path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hesitant to use URL(fileURLWithPath: checkoutPath).path because Xcode said it's going to be deprecated in the future. What should I do in the future when something is going to be deprecated?

Also, I've updated the code to use .path instead of the pattern I was using. Thank you for the advice!


self.sourceServiceBaseURL = sourceServiceBaseURL
self.formatLineNumber = formatLineNumber
}

/// Formats a local source file URL to a URL hosted by the remote source code service.
/// - Parameters:
/// - sourceFileURL: The location of the source file on disk.
Expand All @@ -45,7 +52,7 @@ public struct SourceRepository {
guard sourceFileURL.path.hasPrefix(checkoutPath) else {
return nil
}

let path = sourceFileURL.path.dropFirst(checkoutPath.count).removingLeadingSlash
return sourceServiceBaseURL
.appendingPathComponent(path)
Expand All @@ -65,7 +72,7 @@ public extension SourceRepository {
formatLineNumber: { line in "L\(line)" }
)
}

/// Creates a source repository hosted by the GitLab service.
/// - Parameters:
/// - checkoutPath: The path of the local checkout.
Expand All @@ -77,7 +84,7 @@ public extension SourceRepository {
formatLineNumber: { line in "L\(line)" }
)
}

/// Creates a source repository hosted by the BitBucket service.
/// - Parameters:
/// - checkoutPath: The path of the local checkout.
Expand All @@ -89,7 +96,7 @@ public extension SourceRepository {
formatLineNumber: { line in "lines-\(line)" }
)
}

/// Creates a source repository hosted by the device's filesystem.
///
/// Use this source repository to format `doc-source-file://` links to files on the
Expand All @@ -98,7 +105,7 @@ public extension SourceRepository {
/// This source repository uses a custom scheme to offer more control local source file navigation.
static func localFilesystem() -> SourceRepository {
SourceRepository(
checkoutPath: "",
checkoutPath: "/",
Anthony-Eid marked this conversation as resolved.
Show resolved Hide resolved
// 2 slashes to specify an empty authority/host component and 1 slash to specify a base path at the root.
sourceServiceBaseURL: URL(string: "doc-source-file:///")!,
formatLineNumber: { line in "L\(line)" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ import SwiftDocC
/// Command-line arguments for specifying the catalog's source repository information.
public struct SourceRepositoryArguments: ParsableArguments {
public init() {}

/// The root path on disk of the repository's checkout.
@Option(
help: ArgumentHelp(
"The root path on disk of the repository's checkout."
)
)
public var checkoutPath: String?

/// The source code service used to host the project's sources.
///
/// Required when using `--source-service-base-url`. Supported values are `github`, `gitlab`, and `bitbucket`.
Expand All @@ -36,7 +36,7 @@ public struct SourceRepositoryArguments: ParsableArguments {
)
)
public var sourceService: String?

/// The base URL where the source service hosts the project's sources.
///
/// Required when using `--source-service`. For example, `https://github.com/my-org/my-repo/blob/main`.
Expand Down Expand Up @@ -81,7 +81,10 @@ extension SourceRepository {
guard let sourceServiceBaseURL = URL(string: sourceServiceBaseURL), sourceServiceBaseURL.scheme != nil, sourceServiceBaseURL.host != nil else {
throw ValidationError("Invalid URL '\(sourceServiceBaseURL)' for '--source-service-base-url' argument.")
}





switch sourceService.lowercased() {
case "github":
self = .github(checkoutPath: checkoutPath, sourceServiceBaseURL: sourceServiceBaseURL)
Expand All @@ -94,6 +97,10 @@ extension SourceRepository {
"Unsupported source service '\(sourceService)'. Use 'github', 'gitlab', or 'bitbucket'."
)
}

guard FileManager.default.directoryExists(atPath: checkoutPath) else {
throw ValidationError("Checkout path directory '\(checkoutPath)' doesn't exist for --checkout-path argument.")
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ or use Xcode's _Build Documentation_ command.

Alternatively, use the `docc` command-line tool directly, for example:

```shell
```shell
docc convert MyNewPackage.docc \
--fallback-display-name MyNewPackage \
--fallback-bundle-identifier com.example.MyNewPackage \
Expand Down Expand Up @@ -61,28 +61,25 @@ service configuration flags like so:
docc convert […] \
--source-service github \
--source-service-base-url https://github.com/<org>/<repo>/blob/<branch> \
--checkout-path <absolute path to local checkout>
--checkout-path <absolute or relative path to local checkout>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't find it necessary to explicitly mention that both absolute and relative paths are supported

Suggested change
--checkout-path <absolute or relative path to local checkout>
--checkout-path <path to local checkout>

```

**GitLab**
```bash
docc convert […] \
--source-service gitlab \
--source-service-base-url https://gitlab.com/<org>/<repo>/-/tree/<branch> \
--checkout-path <absolute path to local checkout>
--checkout-path <absolute or relative path to local checkout>
```

**BitBucket**
```bash
docc convert […] \
--source-service bitbucket \
--source-service-base-url https://bitbucket.org/<org>/<repo>/src/<branch> \
--checkout-path <absolute path to local checkout>
--checkout-path <absolute or relative path to local checkout>
```

> Note: The option `--checkout-path` expects an absolute path to where the package
is checked out, not a relative path.

These arguments can also be provided to `swift package generate-documentation`
if you're using the SwiftPM DocC Plugin or via the `OTHER_DOCC_FLAGS` build
setting when building in Xcode.
Expand Down Expand Up @@ -115,22 +112,22 @@ https://www.example.com/documentation/MyNewPackage/MyNewProtocol

#### Host a Documentation Archive with a File Server

You can host documentation archives you create with DocC from the Swift 5.7
toolchain and later using a regular file server. By default, the server hosts
the documentation at the root of the website, like the "MyNewPackage" example
above. To host the documentation at a specific subpath, pass a custom hosting
You can host documentation archives you create with DocC from the Swift 5.7
toolchain and later using a regular file server. By default, the server hosts
the documentation at the root of the website, like the "MyNewPackage" example
above. To host the documentation at a specific subpath, pass a custom hosting
base path for the `--hosting-base-path` option when you build the documentation
archive.
archive.

```shell
```shell
docc convert MyNewPackage.docc \
--additional-symbol-graph-dir .build \
--output-dir MyNewPackage.doccarchive \
--hosting-base-path MyProject/Base/Path
--hosting-base-path MyProject/Base/Path
```

DocC adds the provided hosting base path before the path of each documentation
page. For example, the URL to view for `MyNewProtocol` protocol in the
page. For example, the URL to view for `MyNewProtocol` protocol in the
`MyNewPackage` documentation might resemble the following:

```
Expand All @@ -147,9 +144,9 @@ the `DOCC_HOSTING_BASE_PATH` build setting when building documentation in Xcode.

#### Host a Documentation Archive Using Custom Routing

A file server is the recommended solution to host your documentation. But, if
A file server is the recommended solution to host your documentation. But, if
you need more control over how the server hosts your content, you can configure
the request routing of your web server so it responds to documentation requests
the request routing of your web server so it responds to documentation requests
with the data and assets within the documentation archive.

> Note: The following sections use Apache as an example. Other web server
Expand Down Expand Up @@ -186,7 +183,7 @@ RewriteRule .* MyNewPackage.doccarchive/$0 [L]
```

With these rules in place, the web server provides access to the contents of
the documentation archive.
the documentation archive.

After configuring your web server to host a documentation archive, keep it up
to date by using a continuous integration workflow that builds the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,35 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
withExtension: "docc",
subdirectory: "Test Bundles"
)!

private let testTemplateURL = Bundle.module.url(
forResource: "Test Template",
withExtension: nil,
subdirectory: "Test Resources"
)!

func testSourceRepositoryAllArgumentsSpecified() throws {
let tempDir = try createTemporaryDirectory(pathComponents: "addFileToCreateValidDirectory")

// removing file:// prefix from checkout path because the directory is not
// recognized as a valid directory with it
let checkoutPath = tempDir.absoluteString
let startIndex = checkoutPath.index(checkoutPath.startIndex, offsetBy: 7)
let absoluteCheckoutPath = String(checkoutPath[startIndex...])


for sourceService in ["github", "gitlab", "bitbucket"] {
try assertSourceRepositoryArguments(
checkoutPath: "checkout path",
checkoutPath: absoluteCheckoutPath,
Comment on lines -33 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

If you pass the original value ("checkout path") here, then you can use this test to verify that a relative path is transformed into an absolute path. Currently the test would pass even without the changes to the SourceRepository initializer.

sourceService: sourceService,
sourceServiceBaseURL: "https://example.com/path/to/base"
) { action in
XCTAssertEqual(action.sourceRepository?.checkoutPath, "checkout path")
XCTAssertEqual(action.sourceRepository?.checkoutPath, "\(absoluteCheckoutPath)/")
XCTAssertEqual(action.sourceRepository?.sourceServiceBaseURL.absoluteString, "https://example.com/path/to/base")
}
}
}

func testDoesNotSetSourceRepositoryIfBothCheckoutPathAndsourceServiceBaseURLArgumentsAreMissing() throws {
try assertSourceRepositoryArguments(
checkoutPath: nil,
Expand All @@ -50,6 +59,28 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
}
}

func testThrowsValidationErrorWhenCheckoutPathIsInvalid() throws {
let tempDir = try createTemporaryDirectory(named: "tmp").appendingPathComponent("InvalidDirectory", isDirectory: false)
let absoluteCheckoutPath = tempDir.absoluteString
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this test if verifying that using a file:// prefix for the command line argument raises an error. AFAIK we don't do that for any other path argument and I don't think it's all that likely that developers will do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That test is checking that an error is thrown when a path is used as an argument, but the directories of the path don't exist.

Specifically, it's testing this guard I added in SourceRepositoryArguments.swift

guard FileManager.default.directoryExists(atPath: checkoutPath) else {
    throw ValidationError("Checkout path directory '\(checkoutPath)' doesn't exist for --checkout-path argument.")
}

To the end of the SourceRepository.init?(from arguments: SourceRepositoryArguments) function


for sourceService in ["github", "gitlab", "bitbucket"] {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
checkoutPath: absoluteCheckoutPath,
sourceService: sourceService,
sourceServiceBaseURL: "https://example.com/path/to/base"
)
) { error in
XCTAssertEqual(
(error as? ValidationError)?.message,
"""
Checkout path directory '\(absoluteCheckoutPath)' doesn't exist for --checkout-path argument.
"""
)
}
}
}

func testThrowsValidationErrorWhenSourceServiceIsSpecifiedButNotSourceServiceBaseURL() throws {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
Expand All @@ -67,7 +98,7 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
)
}
}

func testThrowsValidationErrorWhenSourceServiceBaseURLIsSpecifiedButNotSourceService() throws {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
Expand All @@ -85,7 +116,7 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
)
}
}

func testThrowsValidationErrorWhenSourceServiceBaseURLIsInvalid() throws {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
Expand All @@ -100,7 +131,7 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
)
}
}

func testThrowsValidationErrorWhenCheckoutPathIsNotSpecified() throws {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
Expand All @@ -118,7 +149,7 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
)
}
}

func testThrowsValidationErrorWhenSourceServiceIsInvalid() throws {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
Expand All @@ -133,15 +164,15 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
)
}
}

private func assertSourceRepositoryArguments(
checkoutPath: String?,
sourceService: String?,
sourceServiceBaseURL: String?,
assertion: ((ConvertAction) throws -> Void)? = nil
) throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)

var arguments: [String] = [testBundleURL.path]
if let checkoutPath {
arguments.append(contentsOf: ["--checkout-path", checkoutPath])
Expand All @@ -152,9 +183,9 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
if let sourceServiceBaseURL {
arguments.append(contentsOf: ["--source-service-base-url", sourceServiceBaseURL])
}

let convertOptions = try Docc.Convert.parse(arguments)

let result = try ConvertAction(fromConvertCommand: convertOptions)
try assertion?(result)
}
Expand Down