Linting dapp-agoric-basics #67
Conversation
mujahidkay
force-pushed
the
type-enforcing-dapp-contracts
branch
3 times, most recently
from
3396f5e to
731c00a
Compare
mujahidkay
force-pushed
the
type-enforcing-dapp-contracts
branch
4 times, most recently
from
7e7a9fd to
2ff18df
Compare
contract/package.json
Outdated
There was a problem hiding this comment.
I was confused about the first commit being Update yarn.lock when I didn't see anything that would cause it to update.
Please squash that into this commit that makes the yarn.lock change
| @@ -59,7 +59,7 @@ const makeTranslationTable = (makeSlot, makeVal) => { | |||
| return harden({ convertValToSlot, convertSlotToVal }); | |||
| }; | |||
|
|
|||
| /** @type {(slot: string, iface: string | undefined) => any} */ | |||
| /** @type {(slot: WildSlot, iface: string | undefined) => any} */ | |||
There was a problem hiding this comment.
I appreciate the separated commits but the ordering is confusing. This type hasn't been defined yet.
Looking ahead I see its string | null. I think we should stick with that. I have no idea what a "wild slot" is. Sounds fun tho
contract/scripts/deploy-contract.js
Outdated
There was a problem hiding this comment.
fix causes a version bump an entry in the changelog that says 'Bug Fix".
Linting is never a bug. Use lint: or chore: for such changes
There was a problem hiding this comment.
I'll rebase and adjust all commits accordingly. Thanks!
|
@turadg The last two commits are just cleanups. I will rebase and squash commits before merging the PR (as per your suggestion). |
| /** | ||
| * The null slot indicates that identity is not intended to be preserved. | ||
| * | ||
| * @typedef { string | null } WildSlot |
There was a problem hiding this comment.
oic, WildSlot predates this PR.
I think it's helpful to have this type named so we can keep the explanation about what null indicates.
To find a better name I went searching in agoric-sdk and it turns out that also has WildSlot.
https://github.com/Agoric/agoric-sdk/blob/2d48e143f54911897e8345e1094ae6d19599bef2/multichain-testing/tools/marshalTables.js#L15
@dckc what's the idea behind the name? WDYT of something like OptionalSlot or MaybeSlot or BoardSlot?
There was a problem hiding this comment.
"is not intended to be preserved" maybe TempSlot as in, its temporary. I'm fine with whatever you agree on and decide.
There was a problem hiding this comment.
I suppose WildSlot meant "either a slot or a wildcard". I like MaybeSlot. I'm a fan of haskell and the Typeclassopedia. But when in Rome... I just checked some typescript docs; there I see "optional". So OptionalSlot, please.
mujahidkay
force-pushed
the
type-enforcing-dapp-contracts
branch
from
d6c3167 to
65b0ef7
Compare
mujahidkay
force-pushed
the
type-enforcing-dapp-contracts
branch
from
65b0ef7 to
83d7b03
Compare
Description
This PR enables
tscand fixes lint errors for this dapp. Majority of the code is typed now apart from a few instances where I either had to self-define some types or leave them as TO-DOs for future.Have segregated the types of files by commits so can be reviewed commit by commit.