Simplify argument parsing and processing in the plugin

d-ronnqvist commented 1 month ago

Bug/issue #, if applicable:

Summary

This greatly simplifies the processing of command line arguments for the plugin itself, for what the plugin prepares for DocC, and for what there plugin prepares for symbol graph extraction.

Like #87, this PR is in preparation for another change. Whenever I've asked people about how they that I structure my PRs they've said that they prefer multiple smaller single-purpose PRs. I'm trying to do just that with this PR.

The purpose of this change is to prepare the code to make it easy to make a follow up PR that intercepts and customizes the --output-dir argument to allow using it when building documentation for more than one target. A secondary purpose of this change is to have a testable (and tested) foundation for parsing and processing the command line arguments in the plugin.

This is very close to a non-functional change, but there are a few minor improvements:

The updated help text separates symbol graph options/flags into its own group instead of mixing them with the plugin options.
The updated help text also documents the recently added --symbol-graph-minimum-access-level option.
The updated argument processing fixes a handful of minor bugs with parsing values like --some-option=someValue or correctly not parsing literal values after --.

Dependencies

None.

Testing

Nothing in particular. Run bin/tests.

Checklist

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

[x] Added tests
[x] Ran the ./bin/test script and it succeeded
~[ ] Updated documentation if necessary~ Not applicable.

d-ronnqvist commented 1 month ago

@swift-ci please test

d-ronnqvist commented 1 month ago

To make it easier to review this code, here are a handful of things that are worth calling out:

The separation of plugin and symbol graph arguments from the DocC arguments is all in one short file. This file also defines all the plugin and symbol graph arguments. When someone needs to add a new flag they can easily find this file by going to the declaration of any of the other flags and add the new flag in the same file.
- With this, simple construct for extracting arguments for a specific tool, the code that does separates the arguments per tool is easier to follow.
The help text documentation for all the plugin and symbol graph arguments with is in one short file. When someone needs to add help text for new argument they can easily find this file by going to the declaration of any of the other documented flags and add the new help text in the same file. This was in issue in practice where flags adding help text for flags got deferred because of how complex and spread out it was to implement before.

The docc arguments, with default values, are constructed in a single method. This code is very linear and easy to follow. For example, compare

- /// The command-line options required by the `docc` tool.
- private static let requiredOptions: [CommandLineOption] = [
-     .fallbackDisplayName,
-     .fallbackBundleIdentifier,
-     .additionalSymbolGraphDirectory,
-     .outputPath,
- ]
-
- // Build up an array of required command line options by iterating through
- // the options that are required by `docc` and setting their default
- // value based on the given context for this invocation.
- let requiredOptions = Self.requiredOptions.compactMap { option -> RequiredCommandLineOption? in
-     let optionValue: String
-     switch option {
-     case .fallbackDisplayName:
-         optionValue = targetName
-     case .fallbackBundleIdentifier:
-         optionValue = targetName
-     case .additionalSymbolGraphDirectory:
-         optionValue = symbolGraphDirectoryPath
-     case .outputPath:
-         optionValue = outputPath
-     default:
-         // This will throw an assertion when running in tests but allow us to fail
-         // gracefully when running in production.
-         assertionFailure("Unexpected required option: '\(option)'.")
-         return nil
-     }
-
-     return RequiredCommandLineOption(option, defaultValue: optionValue)
- }
-
- // Now that we've formed an array of required command line options, along with
- // their default values, insert them into the existing set of arguments if
- // they haven't already been specified.
- for requiredOption in requiredOptions {
-     doccArguments = requiredOption.insertIntoArgumentsIfMissing(doccArguments)
- }
+ arguments.insertIfMissing(.option(DocCArguments.fallbackDisplayName, value: targetName))
+ arguments.insertIfMissing(.option(DocCArguments.fallbackBundleIdentifier, value: targetName))
+ arguments.insertIfMissing(.option(DocCArguments.additionalSymbolGraphDirectory, value: symbolGraphDirectoryPath))
+ arguments.insertIfMissing(.option(DocCArguments.outputPath, value: outputPath))

(I'll also point out that it's misleading to call these "required" since DocC doesn't require of these flags—it requires either a catalog or a symbol graph directory, but not both and not the other two arguments).

or compare

- var requiredOptions: [RequiredCommandLineOption] {
-     switch self {
-     case .library:
-         return []
-     case .executable:
-         return [
-             RequiredCommandLineOption(
-                 .fallbackDefaultModuleKind,
-                 defaultValue: "Command-line Tool"
-             )
-         ]
-     }
- }

- /// Adds the options required for the target kind into the given set of arguments if
- /// they are not already specified.
- ///
- /// For example, ``executable`` target kinds require a fallback default module kind
- /// to be specified. So, if the given arguments do not already contain
- /// "`--fallback-default-module-kind`", that option will be added along with its default value.
- func addRequiredOptions(to arguments: Arguments) -> Arguments {
-     var arguments = arguments
-     for requiredOption in requiredOptions {
-         arguments = requiredOption.insertIntoArgumentsIfMissing(arguments)
-     }

-     return arguments
- }
-
- // Add any required options that are specific to the kind of target being built
- doccArguments = targetKind.addRequiredOptions(to: doccArguments)
+ switch targetKind {
+ case .library:
+     break
+ case .executable:
+     arguments.insertIfMissing(.option(DocCArguments.fallbackDefaultModuleKind, value: "Command-line Tool"))
+ }

Similarly, the code that prepares the symbol graph options is simpler and easier to follow. Compare

- for customSymbolGraphOption in customSymbolGraphOptions {
-     switch customSymbolGraphOption {
-     case .extendedTypes.positive:
- #if swift(>=5.8)
-         symbolGraphOptions.emitExtensionBlocks = true
- #else
-         print("warning: detected '--include-extended-types' option, which is incompatible with your swift version (required: 5.8)")
- #endif
-     case .extendedTypes.negative:
- #if swift(>=5.8)
-         symbolGraphOptions.emitExtensionBlocks = false
- #else
-         print("warning: detected '--exclude-extended-types' option, which is incompatible with your swift version (required: 5.8)")
- #endif
-     case .skipSynthesizedSymbols:
-         symbolGraphOptions.includeSynthesized = false
-     default:
-         fatalError("error: unknown PluginFlag (\(customSymbolGraphOption.parsedValues.joined(separator: ", "))) detected in symbol graph generation - please create an issue at https://github.com/swiftlang/swift-docc-plugin")
-     }
- }
+ if customSymbolGraphOptions.skipSynthesizedSymbols == true {
+     symbolGraphOptions.includeSynthesized = false
+ }
+ if let includeExtendedTypes = customSymbolGraphOptions.includeExtendedTypes {
+ #if swift(<5.8)
+     print("warning: detected '--\(includeExtendedTypes ? "include" : "exclude")-extended-types' option, which is incompatible with your swift version (required: 5.8)")
+ #else
+     symbolGraphOptions.emitExtensionBlocks = includeExtendedTypes
+ #endif
+ }

The core argument processing utility is well tested in this file.
- Since all the argument processing uses this, there's no need to repeat the same tests for each flag.
- This also fixes some bugs, for example, the old disableIndex flag didn't remove all spellings of the index flag and the old outputPath flag—and all of the other flags—didn't support the --option=value spelling.

sofiaromorales commented 4 weeks ago

@d-ronnqvist - Why are we implementing our own command line parser instead of using swift argument parser?

d-ronnqvist commented 4 weeks ago

@d-ronnqvist - Why are we implementing our own command line parser instead of using swift argument parser?

Swift Argument Parses doesn't solve what the DocC Plugin needs to accomplish;

separating different arguments for different tools (including forwarding any future unknown arguments) to one of the tools.
preparing command line invocations for other tools by inspecting, adding, and removing raw command line flags and options.

d-ronnqvist commented 4 weeks ago

Would it be possible to consolidate all the information about a given argument, i.e. name, spelling, value, default value, help text in one place?

For the flags with documentation I moved everything except the default value into the DocumentedFlag definition in https://github.com/swiftlang/swift-docc-plugin/pull/88/commits/f14201a7fd5f78addde97c5c1830eeadb3f0bf90.

My reason for not moving the default values is that I find that it gets unnecessarily complicated doing so. There are mostly boolean flags but there are also a couple of non-boolean optional arguments (symbol-graph-access-level now and output-path in a follow up PR). I feel that this is an improvement that we can make in the future if we find a need for it then, but for now I prefer to keep the code simpler.

d-ronnqvist commented 4 weeks ago

@swift-ci please test

d-ronnqvist commented 4 weeks ago

@swift-ci please test