-
-
Notifications
You must be signed in to change notification settings - Fork 311
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: fix verification of README code snippets #6868
base: unstable
Are you sure you want to change the base?
Conversation
@@ -54,7 +54,7 @@ describe("web3_provider", function () { | |||
mutateProvider: false, | |||
}); | |||
|
|||
const web3 = new Web3(nonVerifiedProvider); | |||
const web3 = new Web3(verifiedProvider); //TODO: type is not compatible |
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.
@nazarhussain the verifiedProvider
is not compatible with what Web3
expects, and it's not clear to me why you would pass the nonVerifiedProvider
provider as it is not mutated, meaning the prover is not used at all..
Same in the README, the example does not make sense to me.
Performance Report✔️ no performance regression detected Full benchmark results
|
{ | ||
"extends": "../../tsconfig.json", | ||
"compilerOptions": {}, | ||
// `typescript-docs-verifier` uses `ts-node` to compile README snippets |
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.
If you want to add comments in JSON, rename those to .jsoc
extension so it does not highlight as error.
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.
that's really only highlighted this way in the diff, we have comments in other tsconfig files as well
Lines 12 to 15 in 14855ea
// We want to speed up the CI run for all tests, which require us to use the | |
// `transpileOnly` mode for the `ts-node`. This change requires to treat types for each module | |
// independently, which is done by setting the `isolatedModules` flag to `true`. | |
"isolatedModules": true, |
tsconfig itself supports comments as is, might be able to remove this if upstream issue is fixed
@@ -54,7 +54,7 @@ describe("web3_provider", function () { | |||
mutateProvider: false, | |||
}); | |||
|
|||
const web3 = new Web3(nonVerifiedProvider); | |||
const web3 = new Web3(verifiedProvider); //TODO: type is not compatible |
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.
const web3 = new Web3(verifiedProvider); //TODO: type is not compatible | |
const web3 = new Web3(nonVerifiedProvider); |
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.
Because we want to get the accounts via normal RPC and then verify it through our ELRpc
object.
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 examples in the README show otherwise
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.
as per README we pass the verifiedProvider
to Web3
but the types are not compatible, the example in the README does not work
lodestar/packages/prover/README.md
Line 54 in 14855ea
const web3 = new Web3(provider); |
// and hence it is required to disable `transpileOnly` mode as it disables | ||
// all type checks. Set at root to be directly forwarded to `ts-node` until | ||
// https://github.com/bbc/typescript-docs-verifier/issues/31 is resolved | ||
"transpileOnly": 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.
If you want to set this it should go under ts-node
not on the root level.
"ts-node": {
"transpileOnly": true
}
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 made me think if that config is actually have any effect or not.
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 is passed directly to ts-node by the typescript-docs-verifier
module that's why it has to be in root, see code comment and issue bbc/typescript-docs-verifier#31
Motivation
Code snippets in README files are currently not validated, and some are broken. We should make sure that examples can be copy-pasted and are runnable
Description
Fix verification of README code snippets
There is a bug in
typescript-docs-verifier
which requires to have a override tsconfig per project and passts-node
options in root as extends does not work.Workaround can be removed once bbc/typescript-docs-verifier#31 is resolved.
Closes #6300