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

Copy static assets like custom templates before writing out render nodes. #410

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

krilnon
Copy link
Contributor

@krilnon krilnon commented Oct 19, 2022

This copies custom headers and footers into all the index.html files when using --transform-for-static-hosting and
--experimental-enable-custom-templates.

Adds a test for this as well.

Bug/issue #, if applicable: #196

Summary

This fixes an issue where index.html files in the static output didn't have custom headers and footers (aside from the root index.html).

Per Ethan's advice in the originating issue, I moved outputConsumer.consume(assetsInBundle: bundle) further up the file to where some other pregeneration work seemed to be happening.

Dependencies

None.

Testing

You can either observe the output of thetestConvertWithCustomTemplatesForStaticHosting test or run: swift run docc convert --transform-for-static-hosting --experimental-enable-custom-templates SomeDoccBundle.docc on a DocC bundle that has multiple pages of docs and a custom header.html file.

Steps:

  1. Run the command above.
  2. Verify that each page under /documentation has a custom header.

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 (not really necessary here)

…des.

This copies custom headers and footers into all the index.html files
when using --transform-for-static-hosting and
--experimental-enable-custom-templates.

Adds a test for this as well.
@krilnon
Copy link
Contributor Author

krilnon commented Oct 19, 2022

@swift-ci please test

@krilnon
Copy link
Contributor Author

krilnon commented Oct 19, 2022

Not entirely sure what's up with the Linux test failure; it worked in a local Docker run.

If I had to guess, it'd be something around case sensitivity with the output folder for: Folder(name: "TestConvertWithCustomTemplatesForStaticHosting", content: [expectedIndex]), since those are lowercased in the final output.

@ethan-kusters
Copy link
Contributor

@swift-ci please test Linux platform

@krilnon
Copy link
Contributor Author

krilnon commented Oct 22, 2022

@swift-ci please test

@krilnon
Copy link
Contributor Author

krilnon commented Oct 22, 2022

Ah nice, looks like it was indeed case sensitivity for Linux.

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! Just had a small question.

@@ -246,6 +246,9 @@ public struct DocumentationConverter: DocumentationConverterProtocol {
let renderContext = RenderContext(documentationContext: context, bundle: bundle)

try outputConsumer.consume(renderReferenceStore: renderContext.store)

// Copy images, sample files, and other static assets.
try outputConsumer.consume(assetsInBundle: bundle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep the try/catch around this or is it not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the do-catch that I removed? I mostly did that because the problem list it passes around doesn't exist yet now that this call happens earlier. And to match the line above…

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. It looks like this entire method is wrapped in a do/catch to catch these errors and emit them as diagnostics so I think this shouldn't actually affect behavior.

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.

Thank you @krilnon! This looks great.

@@ -246,6 +246,9 @@ public struct DocumentationConverter: DocumentationConverterProtocol {
let renderContext = RenderContext(documentationContext: context, bundle: bundle)

try outputConsumer.consume(renderReferenceStore: renderContext.store)

// Copy images, sample files, and other static assets.
try outputConsumer.consume(assetsInBundle: bundle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. It looks like this entire method is wrapped in a do/catch to catch these errors and emit them as diagnostics so I think this shouldn't actually affect behavior.

@ethan-kusters
Copy link
Contributor

@swift-ci please test

@ethan-kusters
Copy link
Contributor

@swift-ci please test macOS platform

1 similar comment
@ethan-kusters
Copy link
Contributor

@swift-ci please test macOS platform

@ethan-kusters
Copy link
Contributor

Thank you @krilnon! I'm going to go ahead and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SR-15576] [Swift-DocC] Transform-for-static-hosting does not DocC's experimental custom header/footer feature
2 participants