stardoc: auto-dedent rule doc and attribute docs #13403

alexeagle · 2021-04-25T15:25:48Z

Makes them suitable for markdown output.

Ran bazel build src/main/java/com/google/devtools/build/skydoc:skydoc_deploy.jar && cp bazel-bin/src/main/java/com/google/devtools/build/skydoc/skydoc_deploy.jar ../stardoc/stardoc/stardoc_binary.jar

alexeagle · 2021-04-26T14:18:53Z

FYI @c-parsons could use someone's help to review/merge :)

Makes them suitable for markdown output. Fixes bazelbuild#13402

Ran bazel build src/main/java/com/google/devtools/build/skydoc:skydoc_deploy.jar && cp bazel-bin/src/main/java/com/google/devtools/build/skydoc/skydoc_deploy.jar ../stardoc/stardoc/stardoc_binary.jar

tetromino

Thank you for the PR!

Main question is why we need a new dedentDocstring() method - can we use the existing DocstringParser for dedenting non-function docstrings?

Secondary comment - whatever we do to fix rule docstrings, we ought to also do for providers and aspects; so I think we'd want to dedent in FakeStarlarkRuleFunctionsApi.asProviderFieldInfo(), FakeStarlarkRuleFunctionsApi.forProviderInfo(), FakeStarlarkRuleFunctionsApi.aspect().

alexeagle · 2021-05-07T22:07:12Z

Yeah I considered changing the existing DocstringParser class, but of course it has a bunch of fields which are specific to docs with structured sections like Args: and therefore it's probably a bigger change than this. More work to write and more work to review - and given how PRs often don't find anyone with time to review/merge them I didn't want to do all that work up front.

It's also somewhat risky to make deep changes to existing code, but the test coverage here is pretty good so I think the main issue is just author/reviewer time.

Ran bazel build src/main/java/com/google/devtools/build/skydoc:skydoc_deploy.jar && cp bazel-bin/src/main/java/com/google/devtools/build/skydoc/skydoc_deploy.jar ../stardoc/stardoc/stardoc_binary.jar

aiuto · 2021-06-24T04:37:26Z

Friendly ping to all.

alexeagle · 2021-06-25T13:47:05Z

My question for @tetromino is whether you have time to review a big refactoring here and if that's what you want, before I spend time digging in to how is could be done.

src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java

alexeagle · 2021-08-11T21:37:09Z

@tetromino I think the ball is still in your court, I don't want to invest in a refactoring here that doesn't get merged

aiuto · 2021-10-07T21:21:49Z

@tetromino Friendly ping

sgowroji · 2022-04-21T05:46:31Z

Hello @tetromino, Any update on this PR? Please. Thanks!

alexeagle · 2022-06-03T01:26:36Z

🤷🏻‍♂️

aiuto · 2022-06-03T02:28:41Z

@alexeagle Can you rebase to fix the merge conflict?
@brandjon Since Alex is out for a few months, can you take a look?

brandjon · 2022-12-19T17:56:55Z

@tetromino will be back in office next month. We can discuss broader questions of docstring formatting then. In the meantime I'll leave this PR open.

tetromino

I apologize for dropping the ball here and not reviewing this sooner. I somehow operated under the impression that this code was only needed for the legacy stardoc extractor - but obviously both extractors need something like this logic.

So overall I agree with the proposed change, but there are a number of corner case problems (see my comments). I suggest limiting this PR to just adding DocstringUtils.dedentDocstring() and a set of java tests for it to make sure the corner cases are addressed.

As a side effect, limiting the PR in this way would automatically eliminate merge conflicts :)

Then as a followup, we can apply dedentation to everywhere it needs to be applied to (both in the old and new doc extractors, for all documentable entities).

tetromino · 2023-05-25T20:58:26Z

src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java

+      if (firstLine) {
+        description.append(bufLine);
+        firstLine = false;


Why never dedent the first line? Note that you do use the first line for calculating indentation if the first line's indentation level is non-zero.

tetromino · 2023-05-25T21:04:07Z

src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java

+        description.append(bufLine);
+        firstLine = false;
+      } else if (bufLine.trim().isEmpty()) {
+        description.append("\n");


Returning doc unchanged on line 110 implies that all-whitespace lines become empty lines (even if they contain whitespace beyond indentation) if any dedentation is done, but remain unchanged when not dedenting - which is unintuitive behavior.

Would it make more sense to instead dedent all-whitespace lines by up to the same number of spaces as any other line?

tetromino · 2023-05-25T21:07:20Z

src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java

+    do {
+      endOfLineOffset = doc.indexOf("\n", lineOffset);
+      String line = endOfLineOffset < 0 ? doc.substring(lineOffset) : doc.substring(lineOffset, endOfLineOffset + 1);
+      boolean allWhitespace = line.trim().isEmpty();


line.trim() removes tabs (as well as all other characters below 0x20, e.g. windows carriage returns), not just spaces. DocstringParser.getIndentation() counts only spaces.

alexeagle · 2023-06-02T21:45:40Z

Thanks for the review! I'm not sure when I'll get time to return to this, but I'll try to get someone to carry it to the finish.

tetromino · 2023-07-26T23:14:50Z

I have an in-progress change (depends on other changes which have not been submitted yet) which ought to fix trimming/dedenting everywhere (rules, aspects, providers, repo rules, module extensions, and attributes of any of above).

alexeagle · 2023-07-26T23:19:12Z

That's awesome, I've been sad this one is languishing. Thanks for the update!

tetromino · 2023-07-26T23:47:24Z

For reference, it's part of https://bazel-review.googlesource.com/c/bazel/+/225435 (unfortunately, Gerrit squashed other in-progress changes into it).

Documentation processors often require doc strings to be dedented. For example, Markdown interprets indented blocks as code. This means before handing doc strings to any form of API doc processor (and ideally - when storing the doc string), we want to dedent them to a minimal indentation level and trim blank lines. In the Python world, the standard algorithm for doing so is specified in PEP-257. Until now, we dedented multiline function doc strings in DocstringUtils using an algorithm which differed from PEP-257 in a number of corner cases. Meanwhile, all other docs (doc strings for rules, attributes, providers etc.) were not trimmed/dedented at all, despite often containing multiple lines. To fix, we introduce Starlark.trimDocString and use it comprehensively on all doc strings. Whenever possible, we store the docstring in its trimmed form. The one exception is function docstrings, because they are stored at parse time, not eval time; we have to trim them in the accessor method. This change allows us to massively simplify the DocstringUtils parser, since it no longer needs to mix low-level string munging for dedenting/trimming with the task of finding args, returns, and deprecation stanzas. A more comprehensive alternative to #13403 Fixes #13402 PiperOrigin-RevId: 552859517 Change-Id: I225f064c7b38f2fdbf78242d5b4597ec545518d4

google-cla bot added the cla: yes label Apr 25, 2021

alexeagle force-pushed the i13402 branch 2 times, most recently from f58ba31 to bf72961 Compare April 25, 2021 19:25

alexeagle force-pushed the i13402 branch from bf72961 to 810d089 Compare April 25, 2021 19:39

aiuto requested a review from tetromino April 26, 2021 15:35

aiuto added the team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols label Apr 26, 2021

stardoc: auto-dedent rule doc and attribute docs

5312d38

Makes them suitable for markdown output. Fixes bazelbuild#13402

alexeagle force-pushed the i13402 branch from 810d089 to 5312d38 Compare April 28, 2021 22:12

tetromino requested changes May 3, 2021

View reviewed changes

brandjon added team-Build-Language and removed team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols labels Jun 3, 2021

aiuto reviewed Jun 25, 2021

View reviewed changes

src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java Show resolved Hide resolved

philwo force-pushed the master branch from 26cb401 to 168b89b Compare December 2, 2021 18:07

sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 21, 2022

alexeagle mentioned this pull request Jun 3, 2022

Add missing ts_project docstring aspect-build/rules_ts#33

Merged

aiuto requested a review from brandjon June 3, 2022 02:28

brandjon added team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols and removed team-Build-Language labels Nov 2, 2022

brandjon removed the awaiting-review PR is awaiting review from an assigned reviewer label Dec 19, 2022

alexeagle mentioned this pull request Mar 24, 2023

chore: fix spacing of js_library docstrings aspect-build/rules_js#961

Merged

tetromino requested changes May 25, 2023

View reviewed changes

tetromino mentioned this pull request Jul 18, 2023

Do not HTML-escape and use Markdown inline code for defaults bazelbuild/stardoc#161

Merged

alexeagle closed this Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stardoc: auto-dedent rule doc and attribute docs #13403

stardoc: auto-dedent rule doc and attribute docs #13403

alexeagle commented Apr 25, 2021

alexeagle commented Apr 26, 2021

tetromino left a comment

alexeagle commented May 7, 2021

aiuto commented Jun 24, 2021

alexeagle commented Jun 25, 2021

alexeagle commented Aug 11, 2021

aiuto commented Oct 7, 2021

sgowroji commented Apr 21, 2022

alexeagle commented Jun 3, 2022

aiuto commented Jun 3, 2022

brandjon commented Dec 19, 2022

tetromino left a comment

tetromino May 25, 2023

tetromino May 25, 2023

tetromino May 25, 2023

alexeagle commented Jun 2, 2023

tetromino commented Jul 26, 2023 •

edited

Loading

alexeagle commented Jul 26, 2023

tetromino commented Jul 26, 2023

stardoc: auto-dedent rule doc and attribute docs #13403

stardoc: auto-dedent rule doc and attribute docs #13403

Conversation

alexeagle commented Apr 25, 2021

alexeagle commented Apr 26, 2021

tetromino left a comment

Choose a reason for hiding this comment

alexeagle commented May 7, 2021

aiuto commented Jun 24, 2021

alexeagle commented Jun 25, 2021

alexeagle commented Aug 11, 2021

aiuto commented Oct 7, 2021

sgowroji commented Apr 21, 2022

alexeagle commented Jun 3, 2022

aiuto commented Jun 3, 2022

brandjon commented Dec 19, 2022

tetromino left a comment

Choose a reason for hiding this comment

tetromino May 25, 2023

Choose a reason for hiding this comment

tetromino May 25, 2023

Choose a reason for hiding this comment

tetromino May 25, 2023

Choose a reason for hiding this comment

alexeagle commented Jun 2, 2023

tetromino commented Jul 26, 2023 • edited Loading

alexeagle commented Jul 26, 2023

tetromino commented Jul 26, 2023

tetromino commented Jul 26, 2023 •

edited

Loading