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

Enable recursive nested list parsing for docs and add tests #2006

Merged
merged 6 commits into from
May 22, 2024

Conversation

guineveresaenger
Copy link
Contributor

@guineveresaenger guineveresaenger commented May 21, 2024

This pull request adds recursive parsing for nested lists in upstream docs of the format.

* `rules` - (Required) Collection of real time alert rules
  * `type` - (Required) Rule type.
  * `issue_detection_configuration` - (Optional) Configuration for an issue detection rule.
    * `rule_name` - (Required) Rule name.
      * `spec` - A spec for the issue detection configuration rule name.

Due to looking at the index of the bullet point list marker in each such line, this addresses the bug underlying #1935 where blank space in front of a top-level list item is permitted in Markdown, as in this example:

## Argument Reference

The following arguments are supported:

  * `name` - (Required) The name of the service.
  * `description` - (Optional) A human-friendly description of the service.

Please see the schema diff for docs descriptions for pulumi-pagerduty: https://github.com/pulumi/pulumi-pagerduty/compare/guin/sample-docsgen?expand=1

Fixes #1935.
Partial fix for pulumi/pulumi-pagerduty#477.

@guineveresaenger guineveresaenger requested a review from a team May 21, 2024 22:01
Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.97%. Comparing base (67fc311) to head (76aaf34).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2006      +/-   ##
==========================================
+ Coverage   60.82%   60.97%   +0.14%     
==========================================
  Files         332      333       +1     
  Lines       44727    44830     +103     
==========================================
+ Hits        27206    27334     +128     
+ Misses      15998    15972      -26     
- Partials     1523     1524       +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.

This looks great!

What happens if there is a bulleted list without * `property` identifiers? What if some (but not all) elements have property identifiers?

pkg/tfgen/docs_test.go Outdated Show resolved Hide resolved
//nolint:lll
func trackBulletListIndentation(line, name string, tracker []bulletListEntry) []bulletListEntry {
listMarkerRegex := regexp.MustCompile("[-*+]")
listMarkerIndex := listMarkerRegex.FindStringIndex(line)[0]
Copy link
Member

Choose a reason for hiding this comment

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

Will this never be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. It will never be empty because we only run this code if we already detected a bullet listed arg via parseArgFromMarkdownLine.

Copy link
Member

Choose a reason for hiding this comment

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

That would be a great thing to explicitly assert with contract, or at least explain in a comment.

pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
// https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#nested-lists
//
//nolint:lll
func trackBulletListIndentation(line, name string, tracker []bulletListEntry) []bulletListEntry {
Copy link
Member

Choose a reason for hiding this comment

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

This algorithm makes a lot of sense.

pkg/tfgen/docs_test.go Show resolved Hide resolved
@guineveresaenger
Copy link
Contributor Author

guineveresaenger commented May 22, 2024

What happens if there is a bulleted list without * property identifiers?

This code only executes on lines that have already an established property identifier as defined in argumentBulletRegexp. So, nothing happens - that bulleted list would get appended to a previous identifier as extra description lines.

What if some (but not all) elements have property identifiers?

Same story, really - keeping in mind that for the purposes of docsPath detection, we do not need to look at any lines that do not match the argumentBulletRegexp. They should get rendered as any other non-argument-containing line.

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.

LGTM!

//nolint:lll
func trackBulletListIndentation(line, name string, tracker []bulletListEntry) []bulletListEntry {
listMarkerRegex := regexp.MustCompile("[-*+]")
listMarkerIndex := listMarkerRegex.FindStringIndex(line)[0]
Copy link
Member

Choose a reason for hiding this comment

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

That would be a great thing to explicitly assert with contract, or at least explain in a comment.

@guineveresaenger guineveresaenger enabled auto-merge (squash) May 22, 2024 19:44
@guineveresaenger guineveresaenger merged commit 110d030 into master May 22, 2024
11 checks passed
@guineveresaenger guineveresaenger deleted the guin/nested-list-parsing branch May 22, 2024 19:53
@t0yv0
Copy link
Member

t0yv0 commented May 22, 2024

Thank you so much🔥

guineveresaenger added a commit to pulumi/pulumi-pagerduty that referenced this pull request May 24, 2024
Due to [somewhat
nonstandard](https://registry.terraform.io/providers/PagerDuty/pagerduty/latest/docs/resources/service)
block descriptions, we were omitting some fields on pagerduty.Service
doc.

This pull request adds a docEdit rule that strips all of the extraneous
text from the nested block description and rewrites it to reflect the
nested blocks format the bridge expects.

This fixes field descriptions for `ServiceSupportHours`,
`ServiceScheduledAction`, and `ServiceIncidentUrgencyRule` nested
blocks.

Note that this PR is also build off of latest bridge rather than a
release, to incorporate the much larger markdown parsing changes made in
pulumi/pulumi-terraform-bridge#2006.

Further note that this PR drops the additional information contained in
the irregular block descriptions. This is the current status quo for all
Pulumi providers. We additionally do not currently render extra
information in the nested properties sections.

Closes #477.
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.

Bridge does not support all valid Markdown list formats
3 participants