-
Notifications
You must be signed in to change notification settings - Fork 44
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
base: master
Are you sure you want to change the base?
Conversation
…ite code chooser. Remove source replacement for all conversion errors.
…that section if so.
… for all possible combinations
Codecov ReportAttention: Patch coverage is
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. |
pkg/tfgen/installation_docs.go
Outdated
|
||
} | ||
|
||
func checkExamplesEmpty(contentStr string) bool { |
There was a problem hiding this comment.
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:
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.
pkg/tfgen/installation_docs.go
Outdated
|
||
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)) |
There was a problem hiding this comment.
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.
pkg/tfgen/installation_docs.go
Outdated
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 | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 nits:
- Let's operate in
[]byte
space (instead ofstring
) space as much as possible, since that's whatgoldmark
uses. - Postfixing a variable name by its type should be avoided if possible.
- Let's favor returning early if possible.
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 |
There was a problem hiding this comment.
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?
## Example Usage | ||
|
||
|
||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
05fe998
to
eb4af28
Compare
This pull request does the following:
pclExample
object for empty pulumi YAML or PCL explicitly and before attempting to convert, removing the empty string check from the converter functionsexampleUnavailable
placeholder constant for use when conversion fails on an individual languagesuccessfulConversion
flag that flips to True as soon as any one language registers as having content other than theexampleUnavailable
message. The idea is that if even only one pulumi language converted, we want to display the code chooser.Fixes #2819