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

starlark: dedent continued function param lines #12286

Closed
wants to merge 3 commits into from

Conversation

alexeagle
Copy link
Contributor

This allows multi-line parameter documentation under Args: without extra indents.
Fixes bazelbuild/stardoc#74

@google-cla google-cla bot added the cla: yes label Oct 15, 2020
alexeagle pushed a commit to alexeagle/stardoc that referenced this pull request Oct 15, 2020
This allows multi-line parameter documentation under Args: without extra indents.
Fixes bazelbuild/stardoc#74
alexeagle pushed a commit to alexeagle/stardoc that referenced this pull request Oct 15, 2020
@alexeagle
Copy link
Contributor Author

ping @c-parsons are you the right person to merge?

Copy link
Contributor

@c-parsons c-parsons left a comment

Choose a reason for hiding this comment

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

Could you modify pure_markdown_test as well to demonstrate how this change affects markdown tables?

break;
}
String trimmedLine = line.substring(baselineIndentation);
if (lineIndentation < continuationIndentation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not truly finding the minimum indentation value here, as we're still processing as we go.
Consider the difference in behavior for:

  Line one
          Line two
                    Line three

versus

               Line one
        Line two
   Line three

Copy link
Contributor Author

@alexeagle alexeagle Oct 21, 2020

Choose a reason for hiding this comment

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

correct, but the failure mode is that you get some extra whitespace if the continuation is accidentally not consistent. For your example I think

Line one
        Line two
                  Line three

and

Line one
Line two
Line three

are the best outcome we'll get without scanning ahead for the minimal indent. Do you think it's worth adding the complexity to scan ahead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I don't think it's much more complex to scan ahead (we're not dealing with gigantic strings here, so it should be fine to store all the lines in a list, take the min indent, and then iterate over)

I foresee later user confusion / frustration with the current behavior, if we don't fix this right the first time.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, done PTAL

@c-parsons
Copy link
Contributor

FYI @alandonovan @brandjon

@alexeagle
Copy link
Contributor Author

Added the change to pure_markdown_template_test as requested :)

@c-parsons
Copy link
Contributor

FYI @alandonovan

@alexeagle
Copy link
Contributor Author

ping @c-parsons mergey merge-merge?

@c-parsons
Copy link
Contributor

Merge in review. Thanks for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macro documentation has extra indent
4 participants