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

autoformat jsdoc of some packages #4906

Merged
merged 7 commits into from
Jul 21, 2023
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Mar 23, 2022

Description

Enable #4903 just for these packages,

  • ertp
  • inter-protocol
  • vats

There are more formatting options we could consider eventually. This adopts the minimum set to reduce the friction to getting the main value: auto-formatting type annotations.

Security Considerations

--

Documentation Considerations

Until #4900 is complete we'll have to selectively enable paths with the glob.

Testing Considerations

  • Verify that re-reunning yarn format is stable and only affects opt-in packages.
  • verify that yarn prettier in a opt-in path is formatted and others aren't
  • verify that formatOnSave in a opt-in path is formatted and others aren't

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

just getting started. Here are some reactions.

On the whole, I like the changes. I don't understand when it switches commas to semicolons in lists, though.

* pool, taken from the `fromSeat`. Then reallocate over all the seat
* arguments and the rewardPoolSeat.
*
* @callback ReallocateWithFee Transfer the indicated amount to the
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes it harder to distinguish the comment from the name when reading the source. Is there punctuation that can be inserted (' - ' for instance) that makes it more visible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree for code reviews. In an IDE it's still quite readable:
Screen Shot 2022-03-24 at 9 26 08 AM

I don't see an option to change this. I could file one but I'm not sure what to ask for… that an exception be made for callbacks?

I think this would be most in line with JSDoc and our other function comments:

/**
 * Transfer the indicated amount to the vaultFactory's reward pool, taken from
 * the `fromSeat`. Then reallocate over all the seat arguments and the rewardPoolSeat.
 *
 * @callback ReallocateWithFee
 * @param {Amount} amount
 * @param {ZCFSeat} fromSeat
 * @param {ZCFSeat} [otherSeat]
 * @returns {void}
 */

which renders on hover as,
Screen Shot 2022-03-24 at 9 30 07 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice that VSCode does that, but WebStorm doesn't color code the types.js file.

As a matter of fact, I now see that at the call site for reallocateWithFee, WebStorm tells me that the type is ReallocateWithFee, and doesn't give any help on the parameters. (This is at line 589 in vault.js. WS shows no parameter help for manager.reallocateWithFee(, but the adjacent transfer shows a popup of fromSeat, toSeat, fromLoses, toGains, key.) I've been noticing missing parameter help recently, but didn't notice that it was simultaneously present on other calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice that VSCode does that, but WebStorm doesn't color code the types.js file.

That was just the style I was using. It did distinguish and allowed me to make them more distinct.

at the call site for reallocateWithFee, WebStorm tells me that the type is ReallocateWithFee, and doesn't give any help on the parameters.

After some experimentation with @turadg, we found that @callback didn't pass the parameters through, but a direct declaration of the arguments did. I will experiment so we don't have to describe all the parameters at each declaration. (The particular example I was experimenting with was passed through several layers of API to get to where it was being used.)

Comment on lines 45 to 43
* ACTIVE - vault is in use and can be changed
* LIQUIDATING - vault is being liquidated by the vault manager, and cannot be changed by the user
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the presence of the dashes here that told it not to reflow? Did it notice that the dashes lined up, or did it accidentally preserve that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The dashes were lined up before. The indentation told it not to reformat. Though there is a bug to watch out for hosseinmd/prettier-plugin-jsdoc#155

@turadg turadg force-pushed the 4900-autoformat-jsdoc-run-protocol branch 2 times, most recently from f53503d to e54d855 Compare March 24, 2022 16:21
@turadg turadg mentioned this pull request Mar 24, 2022
2 tasks
Comment on lines 26 to 27
* @property {Ratio} interestRate Annual interest rate charged on loans
* @property {Ratio} loanFee The fee (in BasisPoints) charged when opening or
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping the dashes leads to worse readability in the source. Are the dashes not understood as separators by the jsdoc interpreter?

Copy link
Contributor

Choose a reason for hiding this comment

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

I withdraw this. The IDE does a decent job of distinguishing defined names from commentary.

* so targetK equals startK because we square targetY
* targetK = targetX * targetY = desiredRatio * (startK / desiredRatio)
*
* Since startK/endK is less than one, and we have to worry about early loss of
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeated line?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops yeah, bad copy paste in the manual fix commit

@@ -6,12 +6,12 @@ import { AmountMath } from '@agoric/ertp';
import { natSafeMath } from '@agoric/zoe/src/contractSupport/index.js';

/**
* xy <= (x + deltaX)(y - deltaY)
* `Xy <= (x + deltaX)(y - deltaY)`
Copy link
Contributor

Choose a reason for hiding this comment

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

changing from lower case is a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, it both added backticks and changed the case. I don't think it should have done either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Human error: I added the backticks so it wouldn't change the case, but I forgot to undo its case change. Fixed.

@@ -95,32 +93,30 @@ const { quote: q, details: X } = assert;
* The creatorFacet has one method (makeCollectFeesInvitation, which returns
* collected fees to the creator). `handleParamGovernance()` adds internal
* methods used by the contractGovernor. The contractGovernor then has access to
* those internal methods, and reveals the original AMM creatorFacet to its own
* creator.
* those internal methods, and reveals the original AMM creatorFacet to its own creator.
Copy link
Contributor

Choose a reason for hiding this comment

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

The line breaking algorithm is surprising. If the last line can be this long, why didn't it re-flow the previous lines?

 * The creatorFacet has one method (makeCollectFeesInvitation, which returns collected
 * fees to the creator). `handleParamGovernance()` adds internal methods used by the
 * contractGovernor. The contractGovernor then has access to those internal methods, and
 * reveals the original AMM creatorFacet to its own creator.

I've seen other examples above where the last line is visibly longer than those that precede it.

It's a benefit to not have to think about line breaking, but this is a bizarre approach.

Copy link
Member Author

@turadg turadg Mar 24, 2022

Choose a reason for hiding this comment

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

My guess is it has a width tolerance factor to prevent runts (had to look that up).

I'm happy that code formatters mean I don't have to think about line wrapping. =)

Copy link
Member

Choose a reason for hiding this comment

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

TIL runt

Comment on lines 13 to 14
* @param {() => bigint} getProtocolFeeBP - Retrieve governed protocol fee value
* @param {() => bigint} getPoolFeeBP - Retrieve governed pool fee value
Copy link
Contributor

Choose a reason for hiding this comment

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

See! The dashes can be kept.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it doesn't do any dash removal. That was me manually to be consistent with the places we don't use dashes and because the new capitalization provides a distinguishing feature when syntax highlighting isn't available.

@@ -23,7 +23,7 @@ const { details: X, quote: q } = assert;
const trace = makeTracer('IV');

/**
* @file This has most of the logic for a Vault, to borrow RUN against collateral.
* @file This Has most of the logic for a Vault, to borrow RUN against collateral.
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this @file take a keyword before the prose

Copy link
Member Author

@turadg turadg Mar 24, 2022

Choose a reason for hiding this comment

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

this was fixed 8 hours ago before my comment! hosseinmd/prettier-plugin-jsdoc#143 (comment)

018d239

@turadg
Copy link
Member Author

turadg commented Mar 24, 2022

On the whole, I like the changes.

Me too. I'm going to mark this ready for review.

UPDATE: I noticed in VS Code that when the plugin is installed by Yarn that my saveOnFormat is formatting everywhere because it's not getting the --no-plugin-search option in package.json. I think this is a blocker. I'll try to solve before marking for review again.

I don't understand when it switches commas to semicolons in lists, though.

That's for TypeScript fragments within a @typedef, which is more in line with TypeScript conventions. It's also what Prettier would do for plain .ts.

@turadg turadg marked this pull request as ready for review March 24, 2022 23:49
@turadg turadg marked this pull request as draft March 24, 2022 23:55
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

packages/run-protocol/src/interest.js Outdated Show resolved Hide resolved
Comment on lines 49 to 52
* Transfer the indicated amount to the vaultFactory's reward pool, taken from
* the `fromSeat`. Then reallocate over all the seat arguments and the rewardPoolSeat.
*
* @callback ReallocateWithFee
Copy link
Member

Choose a reason for hiding this comment

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

Did it swap the declaration and prose automatically, or did manually intervene? Do they both mean the same thing in jsdoc?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was manual. When the prose is below a tag it is treated as part of the tag and put onto the same line. I.e. @callback ReallocateWithFee Transfer the indicated amount…. IDE renders it the same either way way.

I thought of filing that as a bug but on second thought I figured it was a reasonable thing to keep tags grouped together at the bottom like in other functions. That's the ordering in JSDoc examples of callback.

Thinking through it more, it's better for the tool not to move @typedef or @callback descriptions. I've filed hosseinmd/prettier-plugin-jsdoc#156

* - the absence of one of these implies the opposite, so `newDebt` is the
* future value fo `debt`, as computed based on values after any `await`
* A note on naming convention:
*
Copy link
Member

Choose a reason for hiding this comment

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

Did it insert a blank line here? Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I think because list items followed.

Comment on lines 250 to 236
* @returns {Amount<'nat'>} As if the vault was open at the launch of this
* manager, before any interest accrued
* @see getActualDebAmount
Copy link
Member

Choose a reason for hiding this comment

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

Swapped automatically?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it puts @see at the end

@@ -32,7 +32,7 @@ const amountGT = (left, right) =>
* Apply the feeRatio to the amount that has a matching brand. This used to
* calculate fees in the single pool case.
*
* @param {{ amountIn: Amount, amountOut: Amount}} amounts - a record with two
* @param {{ amountIn: Amount; amountOut: Amount }} amounts - A record with two
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't it remove this hyphen?

Copy link
Member Author

@turadg turadg Mar 25, 2022

Choose a reason for hiding this comment

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

It doesn't do anything with hyphens. I removed some manually for consistency in this package. Rationale being that the capitalization is enough to set it apart from the param name.

@@ -80,13 +80,12 @@ export const calcDeltaYSellingX = (x, y, deltaX) => {
* pool for that asset. swapInReduced calls this with the calculated amountOut
* to find out if less than the offeredAmountIn would be sufficient.
*
* deltaX = (deltaYOverY/(1 - deltaYOverY))*x
* Equivalently: (deltaY / (Y - deltaY )) * x
* DeltaX = (deltaYOverY/(1 deltaYOverY))*x Equivalently: (deltaY / (Y - deltaY )) * x
Copy link
Member

Choose a reason for hiding this comment

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

This lost linebreak expressed intended meaning. Needs to be restored somehow.

@@ -6,12 +6,12 @@ import { AmountMath } from '@agoric/ertp';
import { natSafeMath } from '@agoric/zoe/src/contractSupport/index.js';

/**
* xy <= (x + deltaX)(y - deltaY)
* `xy <= (x + deltaX)(y - deltaY)`
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this was manual. But still curious: Did you do the manual intervention on the input or the output?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the question is whether the formatter would modify it without backticks, yes it would capitalize because jsdocCapitalizeDescription is left at its default of true.

Alternately I can disable that and we can leave capitalization alone. IMO it's better to enable it to force us to notice what is interpreted as prose, because any rendering of the docstring will render it that way.

Though I can also accept that even if we do agree to that, as a matter of migration we tackle that in a separate stage so that just getting all onto this tool happens faster.

@@ -95,32 +93,30 @@ const { quote: q, details: X } = assert;
* The creatorFacet has one method (makeCollectFeesInvitation, which returns
* collected fees to the creator). `handleParamGovernance()` adds internal
* methods used by the contractGovernor. The contractGovernor then has access to
* those internal methods, and reveals the original AMM creatorFacet to its own
* creator.
* those internal methods, and reveals the original AMM creatorFacet to its own creator.
Copy link
Member

Choose a reason for hiding this comment

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

TIL runt

@turadg
Copy link
Member Author

turadg commented Mar 26, 2022

This is blocked on getting the selective opt-in to work. Reported in https://github.com/hosseinmd/prettier-plugin-jsdoc/issues/157

@turadg turadg force-pushed the 4900-autoformat-jsdoc-run-protocol branch from c7f086d to 3944d1d Compare March 30, 2022 19:00
* }} RunStakePublic
* To take out a loan, get an `AttestationMaker` for your address from the
Copy link
Member Author

Choose a reason for hiding this comment

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

@dckc the formatter reads the paragraph following a typedef as being about that typedef and so indents it.

I think this will be fixed by https://github.com/hosseinmd/prettier-plugin-jsdoc/issues/156

@mhofman mhofman removed their request for review April 21, 2022 17:31
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Good idea as far as I’m concerned.

* reserve: ReserveKit,
* invitationAmount: Amount,
* space: any,
* zoe: ZoeService;
Copy link
Member

Choose a reason for hiding this comment

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

semicolons. That’s interesting. Is that consistent with TS?

Copy link
Member

Choose a reason for hiding this comment

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

I believe TSC prefers semicolons

@turadg turadg force-pushed the 4900-autoformat-jsdoc-run-protocol branch from 3944d1d to afc2132 Compare July 17, 2023 23:18
@turadg turadg changed the title autoformat jsdoc of run-protocol package autoformat jsdoc of some packages Jul 17, 2023
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

The style is generally nicer.

My eyes glazed over about half way thru, so I can't vouch for all the details.

I have a vague unease about changing our bundles for non-functional reasons, but I suppose I can go with "the future is longer than the past".

packages/inter-protocol/scripts/manual-price-feed.js Outdated Show resolved Hide resolved
Comment on lines -46 to +47
/** @type {CopyMap<string, {installation: Installation, boardId: string, path?: string}>} */
/**
* @type {CopyMap<
Copy link
Member

Choose a reason for hiding this comment

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

I think this file is dead code. I guess it doesn't hurt to tweak it, though...

Comment on lines +62 to +67
* | {
* offerPrice: Ratio;
* }
* | {
* offerBidScaling: Ratio;
* }
Copy link
Member

Choose a reason for hiding this comment

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

if we write each of these as 1 line instead of 3, will the tool leave them alone?

* Return a set of transfers for atomicRearrange() that distribute
* `unsoldCollateral` and `proceeds` proportionally to each seat's deposited
* amount. Any uneven split should be allocated to the reserve.
* collected from the auction participants is `proceeds`. Return a set of
Copy link
Member

Choose a reason for hiding this comment

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

does @returns make sense here?

* Return a set of transfers for atomicRearrange() that distribute
* `unsoldCollateral` and `proceeds` proportionally to each seat's deposited
* amount. Any uneven split should be allocated to the reserve.
* collected from the auction participants is `proceeds`. Return a set of
Copy link
Member

Choose a reason for hiding this comment

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

again: @returns?

Comment on lines +33 to +34
* | { bidScaling: Pattern; price: undefined }
* | { bidScaling: undefined; price: Ratio }
Copy link
Member

Choose a reason for hiding this comment

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

this answers my earlier question: yes, the tool leaves it alone if we write these as 1 line each instead of 3

@@ -51,7 +54,6 @@ export const prepareScaledBidBook = baggage =>
'scaledBidBook',
undefined,
/**
*
Copy link
Member

Choose a reason for hiding this comment

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

Removing that blank line when it's not used is nice.

Comment on lines -15 to +16
* @param {*} faucetInstallation
* @param {*} stableInitialLiquidity
* @param {any} faucetInstallation
* @param {any} stableInitialLiquidity
Copy link
Member

Choose a reason for hiding this comment

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

rewriting * as any... interesting.

packages/vats/test/bootstrapTests/test-vaults-upgrade.js Outdated Show resolved Hide resolved
Comment on lines +95 to +107
* @property {Ratio | null} startPrice identifies the priceAuthority and price
* @property {Ratio | null} currentPriceLevel the price at the current auction
* tier
* @property {Amount<'nat'> | null} startProceedsGoal The proceeds the sellers
* were targeting to raise
* @property {Amount<'nat'> | null} remainingProceedsGoal The remainder of the
* proceeds the sellers were targeting to raise
* @property {Amount<'nat'> | undefined} proceedsRaised The proceeds raised so
* far in the auction
* @property {Amount<'nat'>} startCollateral How much collateral was available
* for sale at the start. (If more is deposited later, it'll be added in.)
* @property {Amount<'nat'> | null} collateralAvailable The amount of collateral
* remaining
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there another way to get whitespace back here? It very effectively pointed out the parallels in the types, which is now harder to see. (And harder to tell that it was considered salient.)

Copy link
Member Author

Choose a reason for hiding this comment

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

the tool doesn't do vertical alignment. what parallel is lost?

Note that when this is rendered into HTML it will look like the following. (using default theme)
Screenshot 2023-07-18 at 11 36 25 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

When I was looking through the changes, it became clear to me that the reformatter thinks that the only version of the comments that matters (not the primary version, the only version) is what gets rendered for readers. I find the comments themselves useful at times to the maintainer (me).

The original formatting (which is what I look at to maintain it) directs my attention to the fact that there is a semantic difference between values which are | null, | undefined, and unadorned. The laid-out version makes that hard to see.

 * @typedef {object} BookDataNotification
 *
 * @property {Ratio         | null}      startPrice identifies the priceAuthority and price
 * @property {Ratio         | null}      currentPriceLevel the price at the current auction tier
 * @property {Amount<'nat'> | null}      startProceedsGoal The proceeds the sellers were targeting to raise
 * @property {Amount<'nat'> | null}      remainingProceedsGoal The remainder of
 *     the proceeds the sellers were targeting to raise
 * @property {Amount<'nat'> | undefined} proceedsRaised The proceeds raised so far in the auction
 * @property {Amount<'nat'>}             startCollateral How much collateral was
 *    available for sale at the start. (If more is deposited later, it'll be
 *    added in.)
 * @property {Amount<'nat'> | null}      collateralAvailable The amount of collateral remaining
 * 

Copy link
Member Author

Choose a reason for hiding this comment

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

ok I understand. There no option to retain the whitespace and I don't expect there will be since one key value of the tool is normalizing whitespace. It doesn't know when it's intentional or accidental. The way to maintain whitespace is to make it a codeblock, but that can't happen within the typedef.

If this is a show-stopped for you, let's discuss higher bandwidth

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a show-stopped for you, let's discuss higher bandwidth

Not a show-stopper. On the whole, this is an improvement.

IMO, it may not be enough of an improvement to outweigh the control that it gives up, but I seem to be a minority in that viewpoint. I'm okay with that.

packages/inter-protocol/src/vaultFactory/vault.js Outdated Show resolved Hide resolved
@turadg turadg force-pushed the 4900-autoformat-jsdoc-run-protocol branch from afc2132 to fc69584 Compare July 19, 2023 17:00
@turadg turadg requested a review from dckc July 19, 2023 23:04
@turadg turadg marked this pull request as ready for review July 19, 2023 23:05
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

On the whole, this is an improvement.

concur.

@turadg turadg added this pull request to the merge queue Jul 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 21, 2023
@turadg turadg force-pushed the 4900-autoformat-jsdoc-run-protocol branch from 0964816 to f122489 Compare July 21, 2023 01:40
@turadg turadg enabled auto-merge July 21, 2023 01:40
@turadg turadg added this pull request to the merge queue Jul 21, 2023
Merged via the queue into master with commit 284e2db Jul 21, 2023
@turadg turadg deleted the 4900-autoformat-jsdoc-run-protocol branch July 21, 2023 02:37
mhofman pushed a commit that referenced this pull request Aug 7, 2023
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.

6 participants