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

Do not render empty code choosers on generated index page #2822

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

guineveresaenger
Copy link
Contributor

@guineveresaenger guineveresaenger commented Jan 10, 2025

This pull request does the following:

  1. Prevents the generation of code choosers with empty content
  2. Writes a YAML config file with no code choosers if the example is config-only
  3. Checks the pclExample object for empty pulumi YAML or PCL explicitly and before attempting to convert, removing the empty string check from the converter functions
  4. Adds tests to illustrate all four situations:
    • valid provider config + example
    • invalid code
    • valid provider config but no example
    • valid example but no provider config
  5. Adds exampleUnavailable placeholder constant for use when conversion fails on an individual language
  6. Adds a successfulConversion flag that flips to True as soon as any one language registers as having content other than the exampleUnavailable message. The idea is that if even only one pulumi language converted, we want to display the code chooser.

Fixes #2819

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 85.18519% with 8 lines in your changes missing coverage. Please review.

Project coverage is 68.67%. Comparing base (c158e94) to head (eb4af28).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/tfgen/convert_cli.go 0.00% 3 Missing and 1 partial ⚠️
pkg/tfgen/installation_docs.go 92.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2822      +/-   ##
==========================================
+ Coverage   68.65%   68.67%   +0.02%     
==========================================
  Files         325      325              
  Lines       41537    41578      +41     
==========================================
+ Hits        28516    28553      +37     
- Misses      11418    11419       +1     
- Partials     1603     1606       +3     

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


}

func checkExamplesEmpty(contentStr string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

checkExamplesEmpty isn't a super clear name. Could we change the signature to:

Suggested change
func checkExamplesEmpty(contentStr string) bool {
// isMdSectionEmpty returns true if there is a section titled with `title` and the section had no other content.
func isMdSectionEmpty(title string, content []byte) bool {

Also, let's operate on []byte instead of strings, since that's what goldmark uses.


err := ast.Walk(astNode, func(n ast.Node, entering bool) (ast.WalkStatus, error) {
if section, ok := n.(*section.Section); ok && entering {
sectionText := section.Text([]byte(contentStr))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of converting to text and then checking if nothing came out, can we do this with a more structured check:

A section is empty if it has one child (the title). We can then get the content of the title and validate that.

Comment on lines 507 to 517
func removeEmptyExamples(contentStr string) (string, error) {
if checkExamplesEmpty(contentStr) {
mybytes, err := SkipSectionByHeaderContent([]byte(contentStr), func(headerText string) bool {
return headerText == "Example Usage"
})
contentStr = string(mybytes)
return contentStr, err
}
return contentStr, nil

}
Copy link
Member

Choose a reason for hiding this comment

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

3 nits:

  1. Let's operate in []byte space (instead of string) space as much as possible, since that's what goldmark uses.
  2. Postfixing a variable name by its type should be avoided if possible.
  3. Let's favor returning early if possible.
Suggested change
func removeEmptyExamples(contentStr string) (string, error) {
if checkExamplesEmpty(contentStr) {
mybytes, err := SkipSectionByHeaderContent([]byte(contentStr), func(headerText string) bool {
return headerText == "Example Usage"
})
contentStr = string(mybytes)
return contentStr, err
}
return contentStr, nil
}
func removeEmptyExamplesSection(content []byte) ([]byte, error) {
const title = "Example Usage"
if !isMdSectionEmpty(title, content) {
return content
}
isExamplesSection := func(s string) bool { return title == s }
return SkipSectionByHeaderContent(content, isExamplesSection)
}

@@ -0,0 +1,132 @@
This example is invalid and should not be translated for this test to pass
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the example was translated. Do we need a more invalid example?

Comment on lines +3 to +6
## Example Usage



Copy link
Member

Choose a reason for hiding this comment

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

I thought a goal of this PR was to remove empty ## Example Usage sections like this.

Copy link
Contributor Author

@guineveresaenger guineveresaenger Jan 10, 2025

Choose a reason for hiding this comment

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

Yes! This test is only for the code conversion. We would still have an empty Examples section after code conversion.

@guineveresaenger guineveresaenger force-pushed the guin/fix-empty-code-choosers branch from 05fe998 to eb4af28 Compare January 10, 2025 18:18
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.

Ensure that an empty block doesn't have a chooser, ideally rolling up empty heading blocks
2 participants