SR-15314: Update help information for command

[maldahleh]

Bug/issue #, if applicable: SR-15314

Summary

The docc index command expects a data directory from a Documentation Archive, this was not clearly conveyed in the help message, the goal of this PR is to make this a bit more clear.

Dependencies

No dependencies

Testing

Run docc index --help

Index command help

Convert command help

Checklist

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

• Added tests - N/A: No existing test suite, and not sure if it's worth adding.
• Ran the ./bin/test script and it succeeded - No: Some tests are failing but seems unrelated to my PR, created SR-15349
• Updated documentation if necessary

[maldahleh]

I'm not sure about the of JSON files, I considered:

1. omitting it completely
2. current wording
3. using containing instead of of

[d-ronnqvist]

Thanks for opening this PR.

DocumentationBundleOption is used by both the convert action (Convert.swift) and the index action (Index.swift) for its validation and fallback logic. This change would update the help text for both.

Since the convert command is passed a “documentation catalog” (.docc) and the index command is passed a “documentation archive” (.doccarchive) I think that it would be good if these could have different ArgumentHelp information but continue to share the validation and fallback logic.

[maldahleh]

@d-ronnqvist Sorry, I missed that the option was used for both commands, what I did to keep the logic shared was create a new DocumentationOption protocol that extends ParsableArguments and provides the shared logic through an extension. I then created a separate DocumentationArchiveOption to be able to provide different ArgumentHelp.

Does this seem like the correct approach to you? I may be missing language features that could simplify this solution, but this appeared to be the most logical.

I also added screenshots of the help output for both commands in the PR body.

[d-ronnqvist]

Adding a protocol to share this logic looks great. Since it’s a public protocol I would want to document why it exists and what it does. I would also like to find a more specific name for this protocol.

Perhaps something something like this:

/// A parsable argument for an optional directory path.
///
/// This option validates the the provided path exists and that it's a directory.
public protocol DirectoryPathOption: ParsableArguments { ... }

[d-ronnqvist]

I like this idea.

It could be taken further by instead having the ArgumentHelp in the protocol

Suggested change

	var url: URL? { get }
	/// The help information for this directory's `url` argument.
	static var argumentHelp: ArgumentHelp?

and defining the url in the extension:

extension DirectoryPathOption {
   /// The path to a directory.
   @Argument(
       help: argumentHelp,
       transform: URL.init(fileURLWithPath:))
   public var url: URL?
   
   /// The provided ``url`` or the "current directory" if the user didn't provide an argument.
   public var urlOrFallback: URL {
       return url ?? URL(fileURLWithPath: ".")
   }
   
   public mutating func validate() throws { … }
}

[maldahleh]

@d-ronnqvist I like this idea, but Swift doesn't support stored properties in extensions right? From what I can tell, we also can't use wrappers in extensions either.

I was looking around to see if I can find a solution, but I'm not sure if there's a very clean one.

[d-ronnqvist]

Apologies. I though that that would work if the extension was declared in the same file as the protocol but it doesn’t. I tried and even if the url property wasn’t stored it still couldn’t have a property wrapper in the extension. What you’ve done looks like the best solution here.

[d-ronnqvist]

Thank you. This looks great.

The Linux builds are currently failing because of https://bugs.swift.org/browse/SR-15362. Once that has landed in a toolchain I will rerun the tests and merge this PR once the CI has passed.

[d-ronnqvist]

Nice. A symbol link helps put this option in context. 👍

[franklinsch]

@swift-ci test

[maldahleh]

@franklinsch I updated the branch with main but that seems to have wiped the test results, sorry

[franklinsch]

No need to apologize! Triggering a new one:

@swift-ci please test