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

Add an Overview subheader to logically separate Installation and general provider info #2419

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

guineveresaenger
Copy link
Contributor

@guineveresaenger guineveresaenger commented Sep 17, 2024

This pull request is the result of discovering #2420.

This fix:

  • Avoids the rendering issues for unheadered text as outlined in Ensure Markdown renderer preserves newline whitespace #2420 by adding a ## header
  • Visually and logically separates the provider summary text from the installation section.
  • Strips the H1 header earlier in the doc transformation so we can verify remaining content
  • Strips the "schema generated by tfplugindocs" HTML comment

TL;DR: while this is on its face a quick fix, we do want to separate the installation from the general text, so adding the Overview header is something we'd want to do anyway.

Screenshots:

Before:
Screenshot 2024-09-17 at 11 16 15 AM

After:
Screenshot 2024-09-17 at 11 16 34 AM

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 14 lines in your changes missing coverage. Please review.

Project coverage is 57.42%. Comparing base (83d2f45) to head (f998732).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
pkg/tfgen/installation_docs.go 36.36% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2419      +/-   ##
==========================================
- Coverage   57.67%   57.42%   -0.25%     
==========================================
  Files         369      369              
  Lines       50189    50209      +20     
==========================================
- Hits        28948    28835     -113     
- Misses      19664    19796     +132     
- Partials     1577     1578       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

We need to make the overview section conditional on the remaining body not starting with a H2. We don't want to add:

## Overview

## Examples

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Can we add a test that shows we drop ## Overview it has no content.

@guineveresaenger
Copy link
Contributor Author

## Overview doesn't get written if there's no content. https://github.com/pulumi/pulumi-terraform-bridge/pull/2419/files#diff-dbba61023eb9aed059bd34c79844b0f76a82c2a8b296702b9a56cae17ad35a41 should show that but I see how the input doc isn't too descriptive of what the desired behavior is. I've added another test that verifies the output with a bit more context.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

The test still feels pretty funny, but otherwise this looks good.

@@ -107,6 +111,22 @@ func writeInstallationInstructions(goImportBasePath, providerName string) string
)
}

func writeOverviewHeader(content []byte) string {
overviewHeader := "## Overview\n\n"
Copy link
Member

Choose a reason for hiding this comment

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

Go doesn't have much of a const correctness culture, but I think it makes the code much easier to read.

Suggested change
overviewHeader := "## Overview\n\n"
const overviewHeader = "## Overview\n\n"

@@ -107,6 +111,22 @@ func writeInstallationInstructions(goImportBasePath, providerName string) string
)
}

func writeOverviewHeader(content []byte) string {
Copy link
Member

Choose a reason for hiding this comment

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

write implies that the function will write something.

Suggested change
func writeOverviewHeader(content []byte) string {
func getOverviewHeader(content []byte) string {

Comment on lines 123 to 125

func stripSchemaGeneratedByTFPluginDocs(content []byte) []byte {
tfplugindocsComment := regexp.MustCompile("<!-- schema generated by tfplugindocs -->")
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to compile this multiple times.

Suggested change
func stripSchemaGeneratedByTFPluginDocs(content []byte) []byte {
tfplugindocsComment := regexp.MustCompile("<!-- schema generated by tfplugindocs -->")
var tfPluginDocsComment = regexp.MustCompile("<!-- schema generated by tfplugindocs -->")
func stripSchemaGeneratedByTFPluginDocs(content []byte) []byte {

@@ -157,6 +157,57 @@ func TestWriteInstallationInstructions(t *testing.T) {
})
}

func TestWriteOverviewHeader(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd love if this was a bit higher level, probably by the scope of one function. We shouldn't need to add the header to the body in the test content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next level up is testing the whole file conversion. I will add this in another pull request today.
Adding the header to the body here provides more surrounding context, which I was hoping would provide more clarity.

text, err := removeTitle([]byte(contextTest.input))
require.NoError(t, err)
header := writeOverviewHeader(text)
actual := header + string(text)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

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 follows how the surrounding code operates here. It mirrors the context surrounding this function, so we can test on an actual input file, rather than just assert that write/getOverview does the right thing.

@guineveresaenger guineveresaenger enabled auto-merge (squash) September 20, 2024 17:47
@guineveresaenger guineveresaenger merged commit 1cb7594 into master Sep 20, 2024
11 checks passed
@guineveresaenger guineveresaenger deleted the guin/add-overview-subheader branch September 20, 2024 18:05
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.91.1.

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.

3 participants