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

[Anthony-Eid]

Bug/issue #490

Summary

Adds support for relative paths when using --checkout-path as an argument in docc convert cli command.

Paths that start with a slash '/' are assumed to be absolute, otherwise paths are assumed to be relative.

Testing

Updated testSourceRepositoryAllArgumentsSpecified() to use a valid temporary path for SourceRepository checkout path argument.

Created testThrowsValidationErrorWhenCheckoutPathIsInvalid() to make sure that an error is thrown when --checkout-path is an invalid path.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran the ./bin/test script and it succeeded
• Updated documentation if necessary

[Anthony-Eid]

Update: SourceRepository is working with relative paths now! Still need to throw an error if an invalid directory is passed into checkout--path and add a test for that scenario.

[Anthony-Eid]

Is there a better/idomatic way to make temporary directories for unit testing than what I did below?

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: absoluteCheckoutPath,
                sourceService: sourceService,
                sourceServiceBaseURL: "https://example.com/path/to/base"
            ) { action in
                XCTAssertEqual(action.sourceRepository?.checkoutPath, "\(absoluteCheckoutPath)/")
                XCTAssertEqual(action.sourceRepository?.sourceServiceBaseURL.absoluteString, "https://example.com/path/to/base")
            }
        }
    }

Also, is it intentionally that a FileManager.default.directoryExists(atPath: checkoutPath) doesn't consider empty directories as valid? As shown below.

extension FileManagerProtocol {
    /// Returns a Boolean value that indicates whether a directory exists at a specified path.
    package func directoryExists(atPath path: String) -> Bool {
        var isDirectory = ObjCBool(booleanLiteral: false)
        let fileExistsAtPath = fileExists(atPath: path, isDirectory: &isDirectory)
        return fileExistsAtPath && isDirectory.boolValue
    }
}

[d-ronnqvist]

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

[Anthony-Eid]

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!

[d-ronnqvist]

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>

[d-ronnqvist]

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.

[Anthony-Eid]

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

[d-ronnqvist]

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

[d-ronnqvist]

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.

[d-ronnqvist]

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