Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat: New plugin flag for skipping synthesized symbols #28

Merged
merged 6 commits into from
Mar 13, 2023
Merged

Feat: New plugin flag for skipping synthesized symbols #28

merged 6 commits into from
Mar 13, 2023

Conversation

sofiaromorales
Copy link
Contributor

Bug/issue #, if applicable: #27

Summary

This pull request aims to support the generation of archives without synthesized symbols by passing a new plugin flag --skip-synthesized-symbols. This would reduce the size of the generated doccarchive.

The plugin flag --skip-synthesized-symbols has been added to achieve this. It doesn't transform any of the passed arguments but overrides the default value of symbolGraphOptions.includeSynthesized to false. Also, with the proposed implementation, more flags could be added to override other settings of the symbol graph options.

Dependencies

N/A

Testing

For automated testing run the integration tests suite.

For manual testing:

  1. Download the attached package
  2. In Package.swift set the right path for swift-docc-plugin dependency
  3. Run swift package generate-documentation, and swift package generate-documentation --skip-synthesized-symbols, for both compare the files generated at PackageWithConformanceSymbols.doccarchive/data/documentation/packagewithconformancesymbols/bar

PackageWithConformanceSymbols.zip

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

@sofiaromorales
Feat: New plugin flag for skipping synthesized symbols
3eec2d8 
    * Added `--skip-synthesized-symbols` flag as plugin flag to override the symbol graph option `includeSynthesized` and set it to false
    * Array of plugin flags that overrides the defaults of the symbol graph generation
    * Conformance of `PluginFlag` to `Equatable` protocol
    * Unit tests for `--skip-synthesized-symbols`
    * Integration test to make sure that synthesized symbols are excluded from the .doccarchive when `--skip-synthesized-symbols` is passed
ethan-kusters
ethan-kusters reviewed Oct 4, 2022
View reviewed changes
Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thank you! I left some initial feedback and I'll want to do some more testing as well before we land this.

...s/Fixtures/PackageWithConformanceSymbols/Sources/PackageWithConformanceSymbols/FooTest.swift Outdated Show resolved Hide resolved
IntegrationTests/Tests/DocCConvertSynthesizedSymbolsTests.swift Show resolved Hide resolved
Sources/SwiftDocCPluginUtilities/PluginFlags/SkipSynthesizedSymbolsFlag.swift Outdated Show resolved Hide resolved
Sources/SwiftDocCPluginUtilities/PluginFlags/SkipSynthesizedSymbolsFlag.swift Outdated Show resolved Hide resolved
Sources/SwiftDocCPluginUtilities/PluginFlags/SkipSynthesizedSymbolsFlag.swift Outdated Show resolved Hide resolved
@sofiaromorales
fix: Changes requested
b20147a 
* Renamed flag to `--experimental-skip-synthesized-symbols`
* Changed flag description
* Integration test now conforms to `Hashable` protocol instead of `SwiftUI`
@sofiaromorales
Merge branch 'main'
fa7bb7e 
Conflicts:
	Plugins/SharedPackagePluginExtensions/PackageManager+getSymbolGraphsForDocC.swift
	Plugins/Swift-DocC Convert/SwiftDocCConvert.swift
	Plugins/Swift-DocC Preview/SwiftDocCPreview.swift
@ethan-kusters ethan-kusters mentioned this pull request Jan 10, 2023
Merged
3 tasks
theMomax added a commit to theMomax/swift-docc-plugin that referenced this pull request Jan 12, 2023
@theMomax
address review comments with strategy from swiftlang#28
e71e7c3
theMomax added a commit to theMomax/swift-docc-plugin that referenced this pull request Jan 17, 2023
@theMomax
address review comments with strategy from swiftlang#28
7d922f2
@ethan-kusters
Copy link
Contributor

Hi @sofiaromorales would you mind taking a look at the merge conflicts here when you have a chance? I believe #34 introduced a few since it also touches some of this same code.

It would be great to land this in the next release alongside extension support if we can.

@sofiaromorales
Merge branch 'main'
debd20c 
Conflicts:
	IntegrationTests/Package.swift
	Plugins/SharedPackagePluginExtensions/PackageManager+getSymbolGraphsForDocC.swift
	Sources/SwiftDocCPluginUtilities/ParsedArguments.swift
	Tests/SwiftDocCPluginUtilitiesTests/HelpInformationTests.swift
	Tests/SwiftDocCPluginUtilitiesTests/ParsedArgumentsTests.swift
@sofiaromorales
refactor: changes to standardize code with the strategy used in swift…
5e2eb78 
…lang#34
sofiaromorales
sofiaromorales commented Feb 13, 2023
View reviewed changes
Sources/SwiftDocCPluginUtilities/ParsedArguments.swift
@@ -201,7 +201,8 @@ private extension ParsedArguments {
switch self {
case .dumpSymbolGraph:
return [
PluginFlag.extendedTypes
PluginFlag.extendedTypes,
PluginFlag.skipSynthesizedSymbols
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed along the same strategy as in #34 since--experimental-skip-synthesized-symbol is transformed to -skip-synthesized-members flag of the swift package dump-symbol-graph command

@ethan-kusters
Copy link
Contributor

@swift-ci please test

@ethan-kusters
Copy link
Contributor

CC: @theMomax I'm having trouble adding you as reviewer for some reason – but would appreciate your review on this as well.

@theMomax
Copy link
Contributor

@ethan-kusters that's probably because I don't have commit access to Swift repositories and thus cannot formally approve a PR.

As for the review, I think it looks good! One thing I didn't check, though, is whether the symbolGraphOptions.includeSynthesized property has existed since 5.6. I do think so, but otherwise we'd need some special handling for backwards compatibility as I added it in #34.

@heckj heckj mentioned this pull request Feb 24, 2023
Merged
5 tasks
@ethan-kusters
Copy link
Contributor

@swift-ci please test

ethan-kusters
ethan-kusters approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thank you @sofiaromorales!

@ethan-kusters ethan-kusters merged commit 9b12589 into swiftlang:main Mar 13, 2023
@natikgadzhi natikgadzhi mentioned this pull request Jun 4, 2023
Closed
@Kyle-Ye Kyle-Ye linked an issue Aug 13, 2024 that may be closed by this pull request
Support for skipping synthesized symbols #27
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethan-kusters ethan-kusters ethan-kusters approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for skipping synthesized symbols
3 participants
@sofiaromorales @ethan-kusters @theMomax