-
Notifications
You must be signed in to change notification settings - Fork 221
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
refactor: have ESLint manage our JSDocs #1746
Conversation
3b88292
to
f24fcaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The zoe javascript changes look good. I don't know enough about the .json or yarn.locak changes to opine.
d903d91
to
9e76a9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine. How does this relate to #1749?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the removal of spaces around the double ampersand. Why? To me it makes the code substantially harder to read.
@@ -189,11 +191,13 @@ function fail(optDetails = details`Assert failed`) { | |||
throw optDetails.complain(); | |||
} | |||
|
|||
/* eslint-disable jsdoc/require-returns-check,jsdoc/valid-types */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types below failed the linter, mostly due to the incomplete implementation of JSDoc Typescript linting described in #1751.
// @ts-check | ||
|
||
import makeStore from '@agoric/store'; | ||
|
||
/** | ||
* @template T | ||
* @typedef {Object} Device | ||
* @typedef {'Device' & { __deviceType__: T }} Device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the double underbars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly necessary, just to remind people not to try actually using d.__deviceType__
. Do you have a better suggestion?
@@ -6,10 +6,11 @@ | |||
"scripts": { | |||
"build": "exit 0", | |||
"test": "ava", | |||
"lint": "yarn lint:types && eslint '**/*.js'", | |||
"lint": "yarn lint:types&&yarn lint:eslint", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the spaces around the double ampersand? To me, this is a serious legibility problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows experience. Okay, I've reverted that.
9e76a9f
to
01048eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! sorry for the late review
This is much stricter ESLint checking for everything that uses it (except for SwingSet, #1749), but still not as strict as we need: gajus/eslint-plugin-jsdoc#637