Add symbolGraphMinimumAccessLevel option support #82

Kyle-Ye · 2024-06-02T08:23:21Z

Bug/issue #, if applicable:

Close #11

Summary

Add symbolGraphMinimumAccessLevel option support.

The current behavior for it is for library target we assume it to be public and for executable target we assume it to be internal implemented in defaultSymbolGraphOptions.

I propose to add symbol-graph-minimum-access-level option in command line so that we can opt-out the default internal behavior for executable target.

The usage is we want to documentation the executable target via docc bundle but we do not want to expose the internal symbol graph into the documentation.

See apple/swift-mmio#104 (comment)

Dependencies

None

Testing

See ArgumentsSymbolGraphMinimumAccessLevelTests.swift

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

Other Link

Forum proposal: https://forums.swift.org/t/swift-docc-plugin-pitch-add-symbolgraphminimumaccesslevel-option-support/72212

Kyle-Ye · 2024-06-02T08:35:09Z

@swift-ci please test

heckj · 2024-06-02T14:37:18Z

As the individual who opened #11, I'm hugely in favor of this. I don't know what the intended pattern is in calling a command line option "experimental", but since this has a stable part of the API of DocC for some time, and is actively used and adjusted with "OTHER DOCC FLAGS" in a number of places, I'd very much prefer to drop the "experimental" from the CLI option.

Kyle-Ye · 2024-06-02T15:49:30Z

I don't know what the intended pattern is in calling a command line option "experimental"

Oh, I think we'll eventually not need experimental too. It's the suggestion I got from @ethan-kusters on swift-docc Slack channel.

Reply from Slack channel:
There's not - but #11 is tracking the issue. A PR would be great! Maybe we should start with an --experimental prefixed flag? Ideally I think we'd have configuration in the Package.swift itself.

sofiaromorales · 2024-06-03T10:48:41Z

Thanks for opening this PR @Kyle-Ye! I'll be reviewing this in the upcoming days :)

d-ronnqvist · 2024-06-18T13:18:09Z

Sources/SwiftDocCPluginUtilities/Arguments+symbolGraphMinimumAccessLevel.swift

+
+extension Arguments {
+    /// The symbol graph minimum access level, if any, described by this set of command-line arguments.
+    public var symbolGraphMinimumAccessLevel: String? {


Would it make sense for this property to return SymbolGraphOptions.AccessLevel? instead? That way the two call sites wouldn't need to use .flatMap { .init(rawValue: $0) }

I'd like to. But SymbolGraphOptions.AccessLevel is defined in a module we can't access here.

I see. Should we move this into SharedPackagePluginExtensions then so that it can access the necessary types?

This is an alignment with Arguments+outputPath.swift.

If we move this into SharedPackagePluginExtensions , we could not access CommandLineOption anymore unless we move CommandLineOption too or remove CommandLineOption.symbolGraphMinimumAccessLevel dependency.

But in general, I agree your idea here.

Tests/SwiftDocCPluginUtilitiesTests/ArgumentsSymbolGraphMinimumAccessLevelTests.swift

d-ronnqvist · 2024-06-18T13:23:14Z

Sources/SwiftDocCPluginUtilities/CommandLineOptions/CommandLineOption.swift

+
+    /// Specifies the symbol graph minimum access level.
+    static let symbolGraphMinimumAccessLevel = CommandLineOption(
+        defaultName: "--experimental-symbol-graph-minimum-access-level"


One comment and a question:

I don't think this needs to say "experimental".

Should we also define this in a PluginFlag so that it is listed in the swift package generate-documentation --help output as a "plugin option"?

I don't think this needs to say "experimental".

+1 for this.

A PR would be great! Maybe we should start with an --experimental prefixed flag?

What do you think about it? cc @ethan-kusters

Should we also define this in a PluginFlag so that it is listed in the swift package generate-documentation --help output as a "plugin option"?

I will give some check on this part.

I don't think this needs to say "experimental".

Fixed.

Should we also define this in a PluginFlag so that it is listed in the swift package generate-documentation --help output as a "plugin option"?

I remember I tried it at the first time. But I forgot what is stopping me to use it.

rauhul · 2024-07-01T18:01:10Z

Any updates on this? I'd love to adopt it in swift-mmio

Kyle-Ye · 2024-07-02T03:15:17Z

Any updates on this? I'd love to adopt it in swift-mmio

I'll take some time later this week to resolve the feedback I receive from the reviewers.

d-ronnqvist · 2024-07-04T13:21:55Z

Any updates on this? I'd love to adopt it in swift-mmio

I'll take some time later this week to resolve the feedback I receive from the reviewers.

There isn't any blocking feedback. I'd be happy to merge this as is and look at surfacing the new flag in the help text in a later PR.

d-ronnqvist

LGTM

rauhul · 2024-08-09T22:22:47Z

Can we rebase and merge this?

Kyle-Ye · 2024-08-11T17:24:42Z

There isn't any blocking feedback. I'd be happy to merge this as is and look at surfacing the new flag in the help text in a later PR.

Not have time to address all the PR comments. Maybe we can address them later since they are implementation details and there are downstream usage waiting this PR.

Can we rebase and merge this?

Done. I'd like to request @d-ronnqvist for a final approval since I removed experimental prefix.

Kyle-Ye · 2024-08-11T17:24:48Z

@swift-ci please test

d-ronnqvist · 2024-08-12T07:44:40Z

I'll take a look later today

Kyle-Ye · 2024-08-12T07:56:41Z

@swift-ci please test

d-ronnqvist

LGTM

Kyle-Ye force-pushed the feature/kyle/access_level branch from 1c49dd8 to a32a9d8 Compare June 2, 2024 08:33

Kyle-Ye changed the title ~~Add experimental-symbol-graph-minimum-access-level support~~ Add symbolGraphMinimumAccessLevel option support Jun 2, 2024

sofiaromorales self-requested a review June 3, 2024 10:47

d-ronnqvist reviewed Jun 18, 2024

View reviewed changes

d-ronnqvist approved these changes Jul 4, 2024

View reviewed changes

Kyle-Ye force-pushed the feature/kyle/access_level branch from a32a9d8 to 87e893c Compare August 11, 2024 17:22

Kyle-Ye requested a review from d-ronnqvist August 11, 2024 17:25

Kyle-Ye added 3 commits August 12, 2024 15:55

Add symbolGraphMinimumAccessLevel option support

5da19d1

Remove experimental prefix

5ccc886

Fix CR suggestion

22c8086

Kyle-Ye force-pushed the feature/kyle/access_level branch from 87e893c to 22c8086 Compare August 12, 2024 07:56

d-ronnqvist approved these changes Aug 12, 2024

View reviewed changes

Kyle-Ye merged commit d80a059 into swiftlang:main Aug 12, 2024
2 checks passed

Kyle-Ye deleted the feature/kyle/access_level branch August 12, 2024 13:15

Add symbolGraphMinimumAccessLevel option support #82

Add symbolGraphMinimumAccessLevel option support #82

Uh oh!

Conversation

Kyle-Ye commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dependencies

Testing

Checklist

Other Link

Uh oh!

Kyle-Ye commented Jun 2, 2024

Uh oh!

heckj commented Jun 2, 2024

Uh oh!

Kyle-Ye commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sofiaromorales commented Jun 3, 2024

Uh oh!

d-ronnqvist Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

Kyle-Ye Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

Kyle-Ye Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

d-ronnqvist Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

Kyle-Ye Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

Kyle-Ye Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rauhul commented Jul 1, 2024

Uh oh!

Kyle-Ye commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-ronnqvist commented Jul 4, 2024

Uh oh!

d-ronnqvist left a comment

Choose a reason for hiding this comment

Uh oh!

rauhul commented Aug 9, 2024

Uh oh!

Kyle-Ye commented Aug 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kyle-Ye commented Aug 11, 2024

Uh oh!

d-ronnqvist commented Aug 12, 2024

Uh oh!

Kyle-Ye commented Aug 12, 2024

Uh oh!

d-ronnqvist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kyle-Ye commented Jun 2, 2024 •

edited

Loading

Kyle-Ye commented Jun 2, 2024 •

edited

Loading

Kyle-Ye Jun 18, 2024 •

edited

Loading

Kyle-Ye Aug 12, 2024 •

edited

Loading

Kyle-Ye commented Jul 2, 2024 •

edited

Loading

Kyle-Ye commented Aug 11, 2024 •

edited

Loading