-
Notifications
You must be signed in to change notification settings - Fork 8
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
lint devtools, clean-up #16
Conversation
400ad5e
to
5a19ccc
Compare
"typescript": "~5.2.2" | ||
"typescript": "^5.5.3" |
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.
@turadg I had copied versions from agoric-sdk into package.json
; did you update typescript
to "current from npm"?
Did you use any yarn upgrade ...
tooling? or just edit package.json
?
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.
I think this is your change. I don't recall changing this from your branch. It's the version in agoric-sdk
@@ -35,6 +35,7 @@ export const installContract = async ( | |||
* startArgs?: StartArgs; | |||
* issuerNames?: string[]; | |||
* }} opts | |||
* @param {object} [privateArgs] |
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.
I tried to get rid of this copy of privateArgs
. There's already a privateArgs
in startArgs
.
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.
oic. I'll revise
"@endo/ses-ava": "^1.4.0", | ||
"@endo/ses-ava": "^1.2.4", |
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.
this bumps ses-ava down/back, right? I'm interested to know why.
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.
1.2.4 is actually the latest version of https://www.npmjs.com/package/@endo/ses-ava
"@endo/eslint-plugin": "^0.5.2", | ||
"@endo/eslint-plugin": "^2.2.0", |
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.
I'm still learning... how did you choose that version number? is it just the current in npm?
Did anything in particular prompt changing it? Should we generally recommend using the then-current versions of all lint-plugins in Agoric/agoric-sdk#8274 ?
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.
for this one I used yarn upgrade-interactive
@@ -14,8 +14,7 @@ | |||
"build": "yarn build:deployer", | |||
"build:deployer": "rollup -c rollup.config.mjs", | |||
"test": "ava", | |||
"lint": "eslint '**/*.js'", | |||
"lint:types": "tsc -p jsconfig.json", |
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.
we don't do lint:types
any more?
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.
nothing was running lint:types
. agoric-sdk had it that way for backwards compatibility. That scheme requires something like run-s
to lint all the lint:foo
targets.
When you want to lint types, I think yarn tsc
is handy enough. Fewer chars. And it's fast enough that when you want to eslint, it doesn't hurt to wait for tsc first
contract/src/orca.proposal.js
Outdated
@@ -1,29 +1,21 @@ | |||
/* eslint-disable -- FIXME */ | |||
// @ts-check | |||
import { allValues } from './objectTools.js'; | |||
// @ts-nocheck -- FIXME |
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.
this fixes some type stuff but then punts on type checking?
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.
yeah I timed out
contract/tsconfig.json
Outdated
"checkJs": true, | ||
"strict": false, | ||
"noImplicitAny": false, | ||
"useUnknownInCatchVariables": false, |
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.
this looks like some hard-won knowledge.
Something to add to Agoric/agoric-sdk#8274 ?
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.
they manifest pretty loudly. useUnknownInCatchVariables
and noImplicitAny
were relaxing strict: true
but there were so many other hurdles I just turned that off (it's off in sdk). I'll remove these two redundant flags
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.
oh, most of the other problems are solved so I'll flip it to true and suppress a few
yarn 4 has the ability built in. removing instead of porting the patches because their base versions aren't being used anymore
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 ;-)
I'd like us to split #10 into some smaller PRs. This is a candidate for a 1st one.
test
/anyTest
yarn lint
in ci