auto-format JSDoc comments #4903

turadg · 2022-03-23T18:33:40Z

closes: #4900

Description

Makes our JSDoc comments consistent and removes the need for any manual formatting or review of such.

These are the options available: https://github.com/hosseinmd/prettier-plugin-jsdoc#options plus jsdocAddDefaultToDescription not documented in hosseinmd/prettier-plugin-jsdoc@c3fb168#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R124

Option	Setting	Reason
`jsdocAddDefaultToDescription`	set to false	redundant with the type information
`jsdocCapitalizeDescription`	left as true	readability distinguishing description from param name

Look at the PR comments in the changes see examples.

Blame history

Format changes are not free. Losing/obscuring git blame info is a significant cost (h/t @dckc )

The upside of consistent formatting I believe outweighs it and I'll be sure to merge the formatting commit as its own style: commit so reader of git blame knows to look back another step in the history.

We can also cull all the style: commits and bulk ignore them. GitLens supports it and Github has a public beta of support for that in the web UI.

Breaking changes

The formatter has one known bug: hosseinmd/prettier-plugin-jsdoc#153

Security Considerations

--

Documentation Considerations

Improves JSDoc readability.

Testing Considerations

Enforced by the yarn lint:format run in CI.

Before merging, run yarn format repeatedly to check that it's stable for the code being merged. (There is an indentation bug we can work around with a qualified fenced code block.)

turadg · 2022-03-23T18:43:26Z

packages/ERTP/src/amountMath.js

@@ -143,11 +144,11 @@ const optionalBrandCheck = (allegedBrand, brand) => {
 };

 /**
- * @template {AssetKind} [K=AssetKind]
+ * @template {AssetKind} [K=AssetKind] Default is `AssetKind`


this is the most surprising change. We can disable it with jsdocAddDefaultToDescription (Added in hosseinmd/prettier-plugin-jsdoc@c3fb168#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R124 but not documented in https://github.com/hosseinmd/prettier-plugin-jsdoc#options)

Does it add any value? If not, yes, please do disable it.

turadg · 2022-03-23T18:46:27Z

packages/ERTP/src/paymentLedger.js

-   * before a deposit
-   * @param {(newPurseBalance: Amount) => void} updatePurseBalance -
-   * commit the purse balance
+   * @param {Amount} currentBalance - The current balance of the purse before a deposit


we could disable jsdocCapitalizeDescription but I think this default is worth keeping

I like it. But it seems to be inconsistently applied. Other places that look identical did not get capitalized.

mhofman

I'm ok leaving the 2 identified package duplicates, but I'd really like to understand why yarn didn't use the existing ones

mhofman · 2022-03-23T19:00:17Z

yarn.lock

+comment-parser@^1.1.4:
+  version "1.3.1"
+  resolved "https://registry.yarnpkg.com/comment-parser/-/comment-parser-1.3.1.tgz#3d7ea3adaf9345594aedee6563f422348f165c1b"
+  integrity sha512-B52sN2VNghyq5ofvUsqZjmk6YkihBX5vMSChmSK9v4ShjKf3Vk5Xcmgpw4o+iIgtrnM/u5FiMpz9VKb8lpBveA==
+


Why, oh why couldn't yarn use the existing comment-parser@1.3.0 for this!?

mhofman · 2022-03-23T19:01:02Z

yarn.lock

+debug@^4.0.0:
+  version "4.3.4"
+  resolved "https://registry.yarnpkg.com/debug/-/debug-4.3.4.tgz#1319f6579357f2338d3337d2cdd4914bb5dcc865"
+  integrity sha512-PRWFHuSU3eDtQJPvnNY7Jcket1j0t5OuOsFzPPzsekD52Zl8qUfFIPEiswXqIvHWGVHOgX+7G/vCNNhehwxfkQ==
+  dependencies:
+    ms "2.1.2"
+


Same here, yarn is supposed to use the existing matching version debug@4.1.1

FUDCo

Nope.

Completely mangles several carefully formatted comments in ways that are completely unacceptable. I found enough examples in a quick scan that it's not worth the effort to do a more detailed reading of the changes. Back to the drawing board.

FUDCo · 2022-03-23T19:13:59Z

packages/SwingSet/src/kernel/kernelSyscall.js

+   *   For example, if the stored keys are: bar baz foocount foopriority joober
+   *   plugh.3 plugh.47 plugh.8 zot
   *
-   * Then the bounds    would iterate over the key sequence
-   * ----------------   -----------------------------------
-   * 'bar', 'goomba'    bar, baz, foocount, foopriority
-   * 'bar', 'joober'    bar, baz, foocount, foopriority
-   * 'foo'              foocount, foopriority
-   * 'plugh.'           plugh.3, plugh.47, plugh.8
-   * 'baz', 'plugh'     baz, foocount, foopriority, joober
-   * 'bar', '~'         bar, baz, foocount, foopriority, joober, plugh.3, plugh.47, plugh.8, zot
+   *   Then the bounds would iterate over the key sequence
+   *
+   *   'bar', 'goomba' bar, baz, foocount, foopriority 'bar', 'joober' bar, baz,
+   *   foocount, foopriority 'foo' foocount, foopriority 'plugh.' plugh.3,
+   *   plugh.47, plugh.8 'baz', 'plugh' baz, foocount, foopriority, joober
+   *   'bar', '~' bar, baz, foocount, foopriority, joober, plugh.3, plugh.47,
+   *   plugh.8, zot


Completely unacceptable mangling here.

I think this example actually proves the worth of the auto-formatter. Without it, the writer of the comment thought they were writing something intelligible but by JSDoc syntax this is how it appears in an IDE:

If we use conformant JSDoc syntax then it renders something readable:

And the auto-formatter maintains that readability.

So I contend it's a feature that the auto-formatter show the author how JSDoc is interpreting what they're written.

+1 I was wondering if there was some appropriate syntax for these cases, also maybe there's a way to make the formatter ignore it in rare cases?

It would make the migration more involved to go back and change all these things but it seems like it would pay off.

If JSDoc can't display a simple comment the way I wrote it I have issues with that too. I really quite unhappy with the slippery slope we've gotten onto, where accessory tooling has increasingly become the tail that wags the dog.

This tool/tail is not wagging us. It's a value to make our codebase more clear and our development more efficient. We can add new options to it or if somehow our needs are so different from all the other users we can fork it.

It seems you're taking exception with JSDoc itself. JSDoc is not just code comments; it is a structured syntax. It has already been adopted so I don't think we should spend time debating whether to use it.

@FUDCo , I am convinced by the view from the IDE. Preserving the look in the source code for text that loses its meaning in the IDE is not a feature.

@turadg , How did you fix the source so that it renders correctly? Please revise this PR with that fix. Thanks.

For almost all cases, I find the reformatted form a definite improvement. I would like to see this move forward.

How did you fix the source so that it renders correctly?

For the pre-formatted I used a fenced code block" backticks. I used "```md" as a hint to the renderer (without it VSCode highlights as JS). Though I discovered that without a qualifier on the code block's syntax (e.g. md) the formatter will convert to indentation (example), which has a stability bug. hosseinmd/prettier-plugin-jsdoc#155

For list items I used Markdown -.

Please revise this PR with that fix. Thanks.

Done, 6a25dfb

FUDCo · 2022-03-23T19:15:57Z

packages/SwingSet/src/kernel/kernelQueue.js

-   *    (log only rejections), or 'panic' (panic the kernel upon a
-   *    rejection).
-   * @returns {string | undefined} the kpid of the sent message's result promise, if any
+   * @param {string} kref Target of the message


There are a gajillion instances of this, but just commenting on one as an exemplar: The two spaces between the parameter name and the text describing it is deliberate formatting and should not be squeezed out (though punctuation of some sort would be an acceptable alternative)

Using two spaces is not JSDoc.

though punctuation of some sort would be an acceptable alternative

Hyphen as an optional separator for params is part of JSDoc. https://jsdoc.app/tags-param.html

I have personally never seen or used the 2 spaces. I have seen the optional - separator, but I've also noticed some tools with subpar support for it

This is a convention we adopted at Electric Communities when JavaDoc (the ur-parent of JSDoc) first came on the scene. The doc formatter can render the parameter name in a different typeface or color, making it easy to visually separate the name from the text that follows it, but when you are looking at the code itself it helps to have some kind of demarcation and this one is very easy to do.

In this case it shouldn't matter that the two spaces thing is not part of JSDoc, because in the context where it matters we're not looking at what JSDoc interpreted (which in this case would be fine) but at what text is actually in the comment itself. It's fine for JSDoc to squeeze out the whitespace since it is using its own formatting rules. What we're talking about here is the source text, which should read as it was written.

In general, anything that automatically rewrites my code without giving me the choice to accept or reject its changes gets a veto from me.

Is there an option to have the reformatter insert these hyphens consistently everywhere? For me that would be a huge improvement. Without it, I also struggle to spot the transition from jsdoc declaration to prose about the declaration.

@FUDCo You had the same initial reaction to prettier doing a canonical reformatting of your source code. Interesting to explore how this case is similar and different.

Is there an option to have the reformatter insert these hyphens consistently everywhere?

I don't see an option to add or remove them. If there were such an option I would make the case for consistently removing the hyphen. IDEs highlight the prose distinctly and the formatter's capitalization rule distinguishes prose from the param name (which almost always start lowercase).

Huh. I thought I remembered that it did not. But I just looked at my own vscode IDE and I see that it does. So I agree. Thanks!

FUDCo · 2022-03-23T19:16:30Z

packages/SwingSet/src/kernel/parseKernelSlots.js

+ * Parse a kernel slot reference string into a kernel slot object: { type:
+ * STRING, // 'object', 'device', or 'promise' id: Nat }


More unacceptable mangling.

Same as above. Without a code block there's no way for tools to know this was intended to be JS.

* Parse a kernel slot reference string into a kernel slot object: * ```js * { * type: STRING, // 'object', 'device', or 'promise' * id: Nat * * } * ```

Master renders like this an IDE:

@turadg How do you want to handle getting these repaired before this PR is merged? After it is merged is too late. But it is an enormous manual job for one person. When I faced a similar dilemma, we formed a checklist of all the directories that needed manual attention and asked for volunteers to sign up to fix each one. With code owners, this should be even more straightforward.

Agreed. I propose we do it incrementally.

Use this PR to determine our target (configuration options, best practices)

Introduce those options in a separate PR that uses overrides to opt-in one package to formatting (autoformat jsdoc of some packages #4906 )

For each package, a codeowner creates a PR that has a) opts their package into jsdoc formatting, b) manually fixes jsdoc that won't be understood by the formatter (i.e. not valid jsdoc) and c) runs yarn format

Once nearly all packages are opted in, remove the overrides so that it applies to the whole repo

If there is agreement to this I'll document in #4900 as a plan.

Very cool! I agree.

I nominate Zoe as the next package. If you'll (@turadg) make the initial pass, then @erights and I will review.

turadg · 2022-03-23T20:43:21Z

packages/SwingSet/src/kernel/vat-loader/manager-helper.js

@@ -194,7 +194,7 @@ function makeManagerKit(

  /**
   * @param {StreamPosition | undefined} startPos
-   * @returns { Promise<number?> } number of deliveries, or null if !useTranscript
+   * @returns {Promise<?number>} Number of deliveries, or null if !useTranscript


this is surprising but valid

Does TS understand it the same way?

Edit:
I personally would prefer an expressive Promise<number | undefined>.
Also why did the description of number -> Number ?

Good question. They're both Number | null.

I was also surprised to see it reformat

/** @param {Foo=} foo */

into

/** @param {Foo} [foo] */

Are these semantically identical? I have a dim memory of someone explaining a difference in meaning, whereupon I have consistently been using the first style because it expresses what I normally intend.

I believe those are syntactically identical. See https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#param-and-returns

Note that the formatter does have a bug that breaks {Foo=} when used as a return value: hosseinmd/prettier-plugin-jsdoc#153. Fortunately the maintainer was quite prompt in addressing! Their solution is not to use the Google Closure syntax (=) but correctly expand it to | undefined. I personally prefer the more concise form but given the numerous syntaxes we're working with maybe it's best not to support Google Closure style.

mhofman · 2022-03-23T20:44:31Z

packages/SwingSet/src/kernel/kernelSyscall.js

   *   `upperBound`, then result value is a pair of undefineds instead,
   *   signalling the end of iteration.
   *
-   * Usage notes:
+   *   Usage notes:


Interesting why it's adding indentation here. I believe the intent was to make this a note for the function definition, but the result of putting it after @returns would make it included in the return value description ?

That's right. IDE sees all this text as description of @returns. I think anything that's about the function as a whole has to go above the tags.

Another example of the formatter revealing to us how the JSDoc will be parsed.

Presumably the IDE would have shown the original text as associated with the @returns, so this is another case of helping the programmer help the IDE?

turadg · 2022-03-23T20:46:13Z

packages/run-protocol/src/vaultFactory/prioritizedVaults.js

@@ -55,7 +53,7 @@ export const makePrioritizedVaults = reschedulePriceCheck => {
  // current high-water mark fires, we reschedule at the new (presumably
  // lower) rate.
  // Without this we'd be calling reschedulePriceCheck() unnecessarily
-  /** @type {Ratio=} */
+  /** @type {Ratio} */


This change does not preserve semantics.

The = for optional is Google Closure syntax, which TypeScript supports.

Though I think it's fair to standardize on JSDoc syntax which does not support =. Instead we'd have to make these Ratio | undefined.

If we want the tool to support the =, here's the function https://github.com/hosseinmd/prettier-plugin-jsdoc/blob/master/src/utils.ts

Having a change of meaning happen automatically is alarming. From your last comment above, we can fix this? Please do and regenerate this PR. Thanks.

mhofman · 2022-03-23T20:49:23Z

One concern is the discrepancy between JSDoc and TS understanding of JSDoc. It cuts both ways. TS supports features that aren't in the basic JSDoc, and JSDoc has annotations and formats that TS doesn't understand.

FUDCo · 2022-03-23T21:12:53Z

BTW, in the context of some the fairly harsh negative comments I've had here, I should say that about 95% of these changes seem to me to be strict improvements and correct a whole bunch of annoying little formatting nits that have bugged me for some time but were too much bother to engage with systemically. What I'd like is to be able to be more selective about which changes we accept.

erights

I made some requests in single comments on conversations from earlier reviews. Altogether, I want to see this move forward, and for us to habitually use this in the same way we use prettier. But this PR can only be merged once it does not lose the meanings that the original sources were trying to convey. If we don't repair this before merge, it will be much harder to repair it after merge.

But it is an enormous manual job for one person. When I faced a similar dilemma, we formed a checklist of all the directories that needed manual attention and asked for volunteers to sign up to fix each one. With code owners, this should be even more straightforward.

turadg linked an issue Mar 23, 2022 that may be closed by this pull request

Auto-format jsdoc comments #4900

Open

2 tasks

turadg force-pushed the 4900-auto-format-jsdoc-comments branch from ffeec13 to bfd0d30 Compare March 23, 2022 18:40

turadg commented Mar 23, 2022

View reviewed changes

turadg marked this pull request as ready for review March 23, 2022 18:48

turadg requested review from erights, Chris-Hibbert, warner, dckc, kriskowal, michaelfig, samsiegart, FUDCo and mhofman as code owners March 23, 2022 18:48

turadg marked this pull request as draft March 23, 2022 18:53

mhofman reviewed Mar 23, 2022

View reviewed changes

FUDCo suggested changes Mar 23, 2022

View reviewed changes

turadg commented Mar 23, 2022

View reviewed changes

mhofman reviewed Mar 23, 2022

View reviewed changes

turadg commented Mar 23, 2022

View reviewed changes

turadg mentioned this pull request Mar 23, 2022

autoformat jsdoc of some packages #4906

Merged

3 tasks

erights requested changes Mar 24, 2022

View reviewed changes

turadg added 4 commits March 24, 2022 07:44

chore: adopt prettier-plugin-jsdoc

7e7ff36

style: manual jsdoc fixes

6a25dfb

style: yarn format with prettier-plugin-jsdoc

4848817

test: repeat yarn format

324a954

turadg force-pushed the 4900-auto-format-jsdoc-comments branch from bfd0d30 to 324a954 Compare March 24, 2022 14:53

turadg mentioned this pull request Mar 24, 2022

Auto-format jsdoc comments #4900

Open

2 tasks

turadg mentioned this pull request Apr 4, 2022

fix: calculate debt correctly with interest Agoric/dapp-treasury#96

Merged

warner removed their request for review April 7, 2022 17:05

turadg mentioned this pull request Apr 20, 2022

feat(swingset-vat): Include alleged bundleID in checkBundle diagnostics #5164

Merged

turadg mentioned this pull request May 3, 2022

feature!: impose a minimum loan size when opening a vault #5235

Merged

rowgraus added the vaults_triage DO NOT USE label Dec 20, 2022

dckc removed their request for review April 12, 2023 01:49

		* Parse a kernel slot reference string into a kernel slot object: { type:
		* STRING, // 'object', 'device', or 'promise' id: Nat }

auto-format JSDoc comments #4903

Are you sure you want to change the base?

auto-format JSDoc comments #4903

Conversation

turadg commented Mar 23, 2022 • edited Loading

Description

Blame history

Breaking changes

Security Considerations

Documentation Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FUDCo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Mar 23, 2022 • edited Loading

FUDCo commented Mar 23, 2022 • edited Loading

erights left a comment • edited Loading

Choose a reason for hiding this comment

turadg commented Mar 23, 2022 •

edited

Loading

turadg Mar 23, 2022 •

edited

Loading

mhofman Mar 23, 2022 •

edited

Loading

turadg Mar 23, 2022 •

edited

Loading

mhofman commented Mar 23, 2022 •

edited

Loading

FUDCo commented Mar 23, 2022 •

edited

Loading

erights left a comment •

edited

Loading