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.

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

Move configuration before context initialization #1031

Merged
merged 13 commits into from
Oct 7, 2024

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Sep 19, 2024

Bug/issue #, if applicable: rdar://136208312

Summary

This is the first of many changes to redefine how documentation contexts are created. This change specifically moves configuration that's intended to be immutable before the context is initialized.

For source compatibility changes we can't make that configuration actually immutable, but we can warn about any code that modifies after the context is created (and after 6.2 we can make them actually immutable).

Future PRs will build on top of this to change how documentation bundles are discovered and provided to DocC. The ultimate goal is to simplify and decouple the documentation build process so that it can be separated into reusable pieces that other commands can use for build-related and rendering-related work.

Dependencies

None

Testing

Nothing in particular. This is not user-facing change.

Checklist

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

  • Added Updated tests
  • Ran the ./bin/test script and it succeeded
  • [ ] Updated documentation if necessary

@d-ronnqvist d-ronnqvist added the code cleanup Code cleanup *without* user facing changes label Sep 19, 2024
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor Author

@d-ronnqvist d-ronnqvist Sep 19, 2024

Choose a reason for hiding this comment

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

The changes in this file are probably the best illustration of what this PR is about.

Before the code looked like

// 1: Create context
    let context = DocumentationContext(dataProvider: workspace, ...)

// 2: Make configuration

    context.externalMetadata.inheritDocs = inheritDocs
    context.externalMetadata.diagnosticLevel = filterLevel

    if let outOfProcessResolver {
        context.externalDocumentationSources[outOfProcessResolver.bundleIdentifier] = outOfProcessResolver
        context.globalExternalSymbolResolver = outOfProcessResolver
    }

    // Later, in another method of another type that the context is passed to ...

// 3: "Register" the data provider
    try workspace.registerProvider(dataProvider, ...)

but now it looks like

Before the code looked like

// 1: Make configuration
    var configuration = DocumentationContext.Configuration()

    configuration.externalMetadata.inheritDocs = inheritDocs
    configuration.externalMetadata.diagnosticLevel = filterLevel

    if let outOfProcessResolver {
        configuration.externalDocumentationConfiguration.sources[outOfProcessResolver.bundleIdentifier] = outOfProcessResolver
        configuration.externalDocumentationConfiguration.globalSymbolResolver = outOfProcessResolver
    }

// 2: Create context
    let context = DocumentationContext(dataProvider: workspace, ..., configuration: configuration)

    // Later, in another method of another type that the context is passed to ...
    
// 3: "Register" the data provider
    try workspace.registerProvider(dataProvider, ...)

The impact of this is that the window where it's possible to set these values is narrowed to a handful of lines in a single function (from the creation of the "configuration" to the creation of the "context").

Previously, one could continue changing these values all the way until the try workspace.registerProvider(dataProvider, ...) call and it was possible to change them after that as well, although that wouldn't have worked correctly because the values would have already been read when registering the data provider.

configuration.externalDocumentationConfiguration.globalSymbolResolver = outOfProcessResolver
}

(context as _DeprecatedConfigurationSetAccess?)?.configuration = configuration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This silences a deprecation warning. We can't get rid of this line without causing breaking changes in case someone passed a context to the ConvertAction initializer (the default value for that parameter is nil).

@@ -114,8 +112,8 @@ class TopicGraphHashTests: XCTestCase {
XCTAssertEqual(taskGroupLinks, [
"doc://org.swift.docc.example/documentation/Test-Bundle/article",
"doc://org.swift.docc.example/documentation/Test-Bundle/article2",
"doc://com.external.testbundle/article",
"doc://com.external.testbundle/article2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is an example of the bug that was possible with the previous code. These "expected" values are actually the unresolved external links.

The bug in this test is that the configuration (the external documentation source) is added after registering the data provider with the workspace and context.

try workspace.registerProvider(dataProvider)
let context = try DocumentationContext(dataProvider: workspace)

// At this point the context has already "built" all documentation
//
// Any changes after this point has no effect. 
// Specifically, any external links have already failed to resolve and the context won't try again.

context.externalDocumentationSources = ["com.external.testbundle" : ExternalReferenceResolverTests.TestExternalReferenceResolver()]


// Add external resolver
context.externalDocumentationSources = ["com.external.testbundle" : TestExternalReferenceResolver()]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another example of a test that with the bug of making configuration changes after the context has "registered" the documentation, thus having no effect and verifying that the links aren't resolved.

@@ -52,18 +52,7 @@ public struct ConvertAction: Action, RecreatingContext {

let sourceRepository: SourceRepository?

private(set) var context: DocumentationContext {
didSet {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This didSet isn't needed because the setter is private and no code path needs to reconfigure the new context.

Comment on lines +192 to +194
for (platform, fallbackPlatform) in DefaultAvailability.fallbackPlatforms where currentPlatforms[platform.displayName] == nil {
currentPlatforms[platform.displayName] = currentPlatforms[fallbackPlatform.displayName]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This loop comes from here.

In the old code it looked like setting the current platforms here did something but that value was actually written over by the DocumentationConverter that the ConvertAction creates at the end of this initializer (and passes a shared reference to the context to).

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

# Conflicts:
#	Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 5d8ace7 into swiftlang:main Oct 7, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the configure-context-before-init branch October 8, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Code cleanup *without* user facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants