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

chore(packages): fix default param lint warnings #8052

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

toliaqat
Copy link
Contributor

refs: #8032

Description

This PR removes the following lint warnings:

GitHub Actions / lint-rest 
Defaults are not permitted on @param

With the recent update of eslint-plugin-jsdoc, we got a change in their rules to warn about this. This link has detail on why they recommend to not use defaults with @param: https://github.com/gajus/eslint-plugin-jsdoc/blob/main/.README/rules/no-defaults.md

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

These changes make the JSDoc documentation less useful IMO (requiring one to look in two places to find both a description of a parameter and its default value). I'd rather have a lint rule enforcing consistency than one requiring us to exclude relevant details, but the former doesn't seem to be available so I'm torn about running with this vs. disabling no-defaults. Would anyone else like to weigh in?

@@ -315,8 +315,8 @@ export function makeFakeWatchedPromiseManager(
* Configure virtual stuff with relaxed durability rules and fake liveslots
*
* @param {object} [options]
* @param {number} [options.cacheSize=3]
* @param {boolean} [options.relaxDurabilityRules=true]
* @param {number} [options.cacheSize]
Copy link
Member

Choose a reason for hiding this comment

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

cacheSize isn't even a thing anymore AFAICT, having been removed by #7138.

Suggested change
* @param {number} [options.cacheSize]

@@ -305,7 +305,7 @@ const reverse = (keywordRecord = {}) => {
* @param {ZCFSeat} fromSeat
* The seat in contractA to take the offer payments from.
*
* @param {ZCFSeat} [toSeat=fromSeat]
* @param {ZCFSeat} [toSeat]
Copy link
Member

Choose a reason for hiding this comment

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

This one should probably stay, unless the function is updated to actually use an ES6 parameter default.

 export const offerTo = async (
   zcf,
   invitation,
   keywordMapping,
   proposal,
   fromSeat,
-  toSeat,
+  toSeat = fromSeat,
   offerArgs,
 ) => {
   if (keywordMapping === undefined) {
     keywordMapping = harden({});
   }
 
-  const definedToSeat = toSeat !== undefined ? toSeat : fromSeat;
-
   const zoe = zcf.getZoeService();
   const mappingReversed = reverse(keywordMapping);
 
   const newKeywords =
     proposal !== undefined
       ? mapKeywords(proposal.give, mappingReversed)
       : harden({});
 
   // the proposal is in the other contract's keywords, but we want to
   // use `proposal.give` to withdraw
   const payments = await withdrawFromSeat(zcf, fromSeat, newKeywords);
 
   // Map to the other contract's keywords
   const paymentsForOtherContract = mapKeywords(payments, keywordMapping);
 
   const userSeatPromise = E(zoe).offer(
     invitation,
     proposal,
     paymentsForOtherContract,
     offerArgs,
   );
 
   const depositedPromiseKit = makePromiseKit();
 
   const doDeposit = async payoutPayments => {
     // after getPayouts(), getFinalAllocation() resolves promptly.
     const amounts = await E(userSeatPromise).getFinalAllocation();
 
     // Map back to the original contract's keywords
     const mappedAmounts = mapKeywords(amounts, mappingReversed);
     const mappedPayments = mapKeywords(payoutPayments, mappingReversed);
-    await depositToSeat(zcf, definedToSeat, mappedAmounts, mappedPayments);
+    await depositToSeat(zcf, toSeat, mappedAmounts, mappedPayments);
     depositedPromiseKit.resolve(mappedAmounts);
   };

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.

2 participants