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

Refactor example manipulation to use an AST #2312

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Aug 14, 2024

This PR converts the example processing section of *tfMarkdownParser.parse to use goldmark and an AST transformer.

@iwahbe iwahbe self-assigned this Aug 14, 2024
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch from 4cb7761 to 7adba83 Compare August 14, 2024 10:31
@iwahbe iwahbe marked this pull request as ready for review August 14, 2024 10:31
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch 3 times, most recently from 0faf5b8 to ded1a4e Compare August 14, 2024 10:45
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 42 lines in your changes missing coverage. Please review.

Project coverage is 57.14%. Comparing base (bdd39dc) to head (5a498cc).

Files with missing lines Patch % Lines
pkg/tfgen/docs.go 89.06% 13 Missing and 1 partial ⚠️
pkg/tfgen/parse/meta/meta.go 65.85% 10 Missing and 4 partials ⚠️
pkg/tfgen/parse/extension.go 88.65% 9 Missing and 2 partials ⚠️
pkg/tfgen/parse/section/section.go 78.57% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2312      +/-   ##
==========================================
+ Coverage   57.08%   57.14%   +0.06%     
==========================================
  Files         368      369       +1     
  Lines       50535    50676     +141     
==========================================
+ Hits        28848    28960     +112     
- Misses      20114    20136      +22     
- Partials     1573     1580       +7     

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

@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch from ded1a4e to cb54f7c Compare August 14, 2024 15:46
@iwahbe
Copy link
Member Author

iwahbe commented Aug 14, 2024

To merge, I think we need to add tests for splicing together external examples (I don't think that situation is under test at all right now). Otherwise I think this is ready to merge.

iwahbe added a commit that referenced this pull request Aug 16, 2024
I'm adding test coverage in preparation for
#2312.
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch 2 times, most recently from bc30c89 to ec67931 Compare August 16, 2024 16:24
pkg/tfgen/parse/meta.go Outdated Show resolved Hide resolved
pkg/tfgen/parse/meta.go Outdated Show resolved Hide resolved
pkg/tfgen/parse/meta.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch 4 times, most recently from bf18fca to ff5b472 Compare August 27, 2024 10:06
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch from ff5b472 to 0ddc693 Compare August 29, 2024 12:12
Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

I'd love to get the typo fixed but this looks great!

pkg/tfgen/docs_test.go Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch from 0ddc693 to c670f83 Compare September 3, 2024 11:23
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch 4 times, most recently from e66ea28 to 3f5492b Compare September 3, 2024 16:15
Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

nice find! I also like that the string renderer is refactored into the extension. 👍

pkg/tfgen/parse/extension.go Show resolved Hide resolved
pkg/tfgen/parse/extension.go Show resolved Hide resolved
The markdown renderer we use doesn't handle tables. Instead, they were rendered as HTML,
which is fine for the registry but a poor experience for the docs embedded into our
SDKs. This commit provides a custom renderer for markdown tables, based on
"github.com/olekukonko/tablewriter" for the actual table formatting.
@iwahbe iwahbe force-pushed the iwahbe/refactor-parse-examples-with-ast branch from 3f5492b to 5a498cc Compare September 3, 2024 17:03
@iwahbe
Copy link
Member Author

iwahbe commented Sep 4, 2024

I ran a downstream test run and reviewed the results. There are still wholes in our test coverage. I saw a bunch of positive changes, which makes me think this PR is worth merging. These include (but are not limited to):

Better parsing removes - - - from GCP (for every resource), inlining footnotes (necessary since we chop up the upstream document), and preventing the ## # Resource: ... pattern we see in many upstream providers.


Unfortunately, the PR also introduces some blocking regressions:

Previously, every title was Title Cased. We have accidentally dropped that behavior.
Links are rendered as absolute, so https://... is now rendered as [https://...](https://...). This is irrelevant to HTML but makes the markdown less readable.

Most concerning, some providers leaked their frontmatter into the upstream docs, for example newrelic and nomad:

@iwahbe iwahbe marked this pull request as draft September 4, 2024 17:08
@iwahbe
Copy link
Member Author

iwahbe commented Sep 4, 2024

Marking as a draft since this contains known regressions.

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.

2 participants