Add --include-extended-types flag #34

theMomax · 2022-11-13T09:12:43Z

Bug/issue #, if applicable: apple/swift-docc#210

Summary

The --include-extended-types flag is transformed to the --emit-extension-block-symbols flag of the swift package dump-symbol-graph command. It is available starting from Swift 5.8.

Dependencies

All dependencies have been merged. For an overview see apple/swift-docc#210 (comment).

Testing

Use a toolchain generated from main or release/5.8 branches after December 21st
Use the base branch of this PR when adding the swift-docc-plugin to your Package.swift:

dependencies: [
    .package(url: "https://github.com/themomax/swift-docc-plugin", branch: "add-extended-types-flag")
]

Add the --include-extended-types flag when running your command

Verifying the basic functionality can be done with this sample repository. Running swift package --disable-sandbox preview-documentation --include-extended-types --target NestedTypes should produce the result shown below:

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:
Swift 5.7.2: .bin/test succeeded
Swift 5.8/main nightly: .bin/test succeeded after removing the --parallel flag from swift test invocations in the script (Running swift test --parallel on any package fails for me with an error similar to the one described here. However, this is an issue with the nightly toolchains I've been getting for a while now.)
Updated documentation if necessary

theMomax · 2023-01-02T13:52:29Z

@ethan-kusters this PR is ready for review now!

ethan-kusters

Looks great! Just added a few initial thoughts.

ethan-kusters · 2023-01-09T22:02:19Z

Plugins/SharedPackagePluginExtensions/SymbolGraphOptions+override.swift

+                print("warning: detected '--emit-extension-block-symbols' option, which is incompatible with your swift version (required: 5.8)")
+#endif
+            default:
+                print("warning: detected unknown dump-symbol-graph option '\(argument)'")


Should we fatalError here? This is a programmer error, right? It shouldn't be possible to receive an unknown flag here?

For the incompatible version I'd prefer to do it the same way as in SwiftPM, i.e. emitting a warning (see apple/swift-package-manager#5892 (comment)).

The typed configuration options you suggested make the default case for unknown flags redundant anyway.

I added the fatalError here, as it becomes relevant again with the approach from #28

Plugins/SharedPackagePluginExtensions/Target+defaultSymbolGraphOptions.swift

Sources/SwiftDocCPluginUtilities/ParsedArguments.swift

ethan-kusters · 2023-01-09T22:08:43Z

Sources/SwiftDocCPluginUtilities/ParsedArguments.swift

@@ -19,10 +19,25 @@ public struct ParsedArguments {
        return arguments.contains("--help") || arguments.contains("-h")
    }

+    /// Returns the arguments that could be passed to `swift package dump-symbol-graph`.
+    ///
+    /// In practice, however we won't invoke the `dump-symbol-graph` command via the command line,


Should we consider a different way to represent this information instead of strings representing the flags since we're going to re-parse them anyway?

I think tying this implementation to swift package dump-symbol-graph is kind of unnecessarily confusing for folks who might be familiar with SwiftPM plugins but not that specific command.

I think that might be a good idea. I gave it a try based on the following protocol:

/// Transforms a set of arguments, transferring information to a typed configuration. protocol ArgumentsTransformer: ArgumentsTransforming { associatedtype Configuration /// Apply the transformation to the given arguments and configuration. func transform(arguments: inout Arguments, configuration: inout Configuration) }

Please take a look and let me know what you think about it. I'm not super happy with the result, but I'm also not really happy with the whole CLI parsing setup in this plugin. I'm also wondering if a ArgumentParser based rewrite wouldn't be favorable in general.

I'm also wondering if a ArgumentParser based rewrite wouldn't be favorable in general.

This is 100% what I'd like to do long-term. We're just blocked on SwiftPM support to allow command plugins to have dependencies: apple/swift-package-manager#5574.

I'm worried this approach with the generic Configuration is over-optimizing for what we need here. I'm curious if you've looked at the approach in #28? It's less extensible but since we're likely going to need to overhaul our argument parsing solution here in the long-term anyway I wonder if something along those lines would be more maintainable in the shorter term.

This is 100% what I'd like to do long-term. We're just blocked on SwiftPM support to allow command plugins to have dependencies: apple/swift-package-manager#5574.

Ah, yeah, I forgot about that :)

I'm worried this approach with the generic Configuration is over-optimizing for what we need here. I'm curious if you've looked at the approach in #28? It's less extensible but since we're likely going to need to overhaul our argument parsing solution here in the long-term anyway I wonder if something along those lines would be more maintainable in the shorter term.

I felt so too, and, now that I've looked at #28, I agree it's the better short-term solution. I adopted the approach, making sure to cause as little merge-conflicts as possible. I only kept the filter(for:) function in ParsedArguments from my approach, as I felt like without it it would be too easy to accidentally have flags passed to docc that are not meant for it.

Sources/SwiftDocCPluginUtilities/ParsedArguments.swift

Sources/SwiftDocCPluginUtilities/PluginFlags/ExtendedTypesFlag.swift

ethan-kusters · 2023-01-09T22:13:06Z

@swift-ci please test

ethan-kusters · 2023-01-10T02:11:51Z

@swift-ci please test

Sources/SwiftDocCPluginUtilities/HelpInformation.swift

ethan-kusters · 2023-01-10T21:13:14Z

Sources/SwiftDocCPluginUtilities/ParsedArguments.swift

@@ -19,10 +19,25 @@ public struct ParsedArguments {
        return arguments.contains("--help") || arguments.contains("-h")
    }

+    /// Returns the arguments that could be passed to `swift package dump-symbol-graph`.
+    ///
+    /// In practice, however we won't invoke the `dump-symbol-graph` command via the command line,


I'm also wondering if a ArgumentParser based rewrite wouldn't be favorable in general.

This is 100% what I'd like to do long-term. We're just blocked on SwiftPM support to allow command plugins to have dependencies: apple/swift-package-manager#5574.

I'm worried this approach with the generic Configuration is over-optimizing for what we need here. I'm curious if you've looked at the approach in #28? It's less extensible but since we're likely going to need to overhaul our argument parsing solution here in the long-term anyway I wonder if something along those lines would be more maintainable in the shorter term.

Sources/SwiftDocCPluginUtilities/ParsedArguments.swift

Sources/SwiftDocCPluginUtilities/PluginFlags/ExtendedTypesFlag.swift

ethan-kusters

Looks great! Thank you so much for your work here – really excited to get this in.

Could you add a of couple integration tests as well before landing this? I put together a patch just as an example (the integration test suite is a little tricky/weird) that I think gives enough basic coverage but feel free to expand from there or discard it entirely:

add-integration-test.patch

git apply add-integration-test.patch

You can open up the sub-package in the swift-docc-plugin/IntegrationTests subdirectory to run the tests. (I think the parent package needs to be closed for things to work.)

theMomax · 2023-01-13T11:53:24Z

@ethan-kusters I expanded your test with a protocol conformance, but I don't think it makes sense to go any further. After all, the tests here should just verify that the plugin invokes the toolchain correctly, which can easily be verified with these simple cases. Anything beyond that should be/is tested in swift-docc and libSymbolGraphGen in the compiler.

ethan-kusters · 2023-01-13T19:19:16Z

@swift-ci please test

ethan-kusters · 2023-01-13T19:19:38Z

@swift-ci please test

ethan-kusters

Looks great! Thank you! 🚀

The --include-extended-types flag is transformed to the --emit-extension-block-symbols flag of the swift package dump-symbol-graph command. It is available starting from Swift 5.8.

ethan-kusters · 2023-01-23T23:54:09Z

@swift-ci please test

daniel-grumberg · 2023-01-30T17:24:37Z

@swift-ci please test

theMomax force-pushed the add-extended-types-flag branch from 82a5a41 to 9f979e0 Compare November 13, 2022 09:16

theMomax changed the title ~~Add --include-extended-types flag~~ Add --include-extended-types flag Nov 13, 2022

theMomax changed the title ~~Add --include-extended-types flag~~ Add --include-extended-types flag Nov 13, 2022

theMomax mentioned this pull request Nov 13, 2022

[SR-15410] Can't document extensions with DocC apple/swift-docc#210

Closed

theMomax force-pushed the add-extended-types-flag branch from 04aed7c to e0b5fa9 Compare December 22, 2022 10:15

theMomax marked this pull request as ready for review December 22, 2022 10:36

ethan-kusters reviewed Jan 9, 2023

View reviewed changes

theMomax force-pushed the add-extended-types-flag branch from 959aa39 to e4a1a48 Compare January 10, 2023 11:57

ethan-kusters reviewed Jan 10, 2023

View reviewed changes

theMomax force-pushed the add-extended-types-flag branch from e4a1a48 to e71e7c3 Compare January 12, 2023 04:52

theMomax requested a review from ethan-kusters January 12, 2023 05:16

ethan-kusters reviewed Jan 13, 2023

View reviewed changes

theMomax requested a review from ethan-kusters January 13, 2023 11:53

ethan-kusters approved these changes Jan 13, 2023

View reviewed changes

theMomax added 4 commits January 17, 2023 18:20

Add --include-extended-types flag

9fb815c

The --include-extended-types flag is transformed to the --emit-extension-block-symbols flag of the swift package dump-symbol-graph command. It is available starting from Swift 5.8.

address review comments with strategy from apple#28

7d922f2

add integration test that uses extended types

ac2c0f0

update valid license header years

a1f5339

theMomax force-pushed the add-extended-types-flag branch from ec963f0 to a1f5339 Compare January 17, 2023 17:42

Merge branch 'main' into add-extended-types-flag

c234e51

ethan-kusters merged commit 1f13ab5 into apple:main Jan 31, 2023

ethan-kusters mentioned this pull request Jan 31, 2023

Feat: New plugin flag for skipping synthesized symbols #28

Merged

3 tasks

sofiaromorales added a commit to sofiaromorales/swift-docc-plugin that referenced this pull request Feb 13, 2023

refactor: changes to standardize code with the strategy used in apple#34

5e2eb78

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --include-extended-types flag #34

Add --include-extended-types flag #34

theMomax commented Nov 13, 2022 •

edited

theMomax commented Jan 2, 2023

ethan-kusters left a comment

ethan-kusters Jan 9, 2023

theMomax Jan 10, 2023

theMomax Jan 12, 2023

ethan-kusters Jan 9, 2023

theMomax Jan 10, 2023

ethan-kusters Jan 10, 2023 •

edited

theMomax Jan 12, 2023

ethan-kusters commented Jan 9, 2023

ethan-kusters commented Jan 10, 2023

ethan-kusters Jan 10, 2023 •

edited

ethan-kusters left a comment •

edited

theMomax commented Jan 13, 2023

ethan-kusters commented Jan 13, 2023

ethan-kusters commented Jan 13, 2023

ethan-kusters left a comment

ethan-kusters commented Jan 23, 2023

daniel-grumberg commented Jan 30, 2023

Add --include-extended-types flag #34

Add --include-extended-types flag #34

Conversation

theMomax commented Nov 13, 2022 • edited

Summary

Dependencies

Testing

Checklist

theMomax commented Jan 2, 2023

ethan-kusters left a comment

Choose a reason for hiding this comment

ethan-kusters Jan 9, 2023

Choose a reason for hiding this comment

theMomax Jan 10, 2023

Choose a reason for hiding this comment

theMomax Jan 12, 2023

Choose a reason for hiding this comment

ethan-kusters Jan 9, 2023

Choose a reason for hiding this comment

theMomax Jan 10, 2023

Choose a reason for hiding this comment

ethan-kusters Jan 10, 2023 • edited

Choose a reason for hiding this comment

theMomax Jan 12, 2023

Choose a reason for hiding this comment

ethan-kusters commented Jan 9, 2023

ethan-kusters commented Jan 10, 2023

ethan-kusters Jan 10, 2023 • edited

Choose a reason for hiding this comment

ethan-kusters left a comment • edited

Choose a reason for hiding this comment

theMomax commented Jan 13, 2023

ethan-kusters commented Jan 13, 2023

ethan-kusters commented Jan 13, 2023

ethan-kusters left a comment

Choose a reason for hiding this comment

ethan-kusters commented Jan 23, 2023

daniel-grumberg commented Jan 30, 2023

theMomax commented Nov 13, 2022 •

edited

ethan-kusters Jan 10, 2023 •

edited

ethan-kusters Jan 10, 2023 •

edited

ethan-kusters left a comment •

edited