-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: add a built-in console task #5592
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
v-next/hardhat/src/internal/builtin-plugins/console/task-action.ts
Outdated
Show resolved
Hide resolved
I did an initial review despite this being a draft. Looking good! I just left some minor comments. |
63a820d
to
eaa4ced
Compare
eaa4ced
to
3533694
Compare
9180bfe
to
b02caf9
Compare
5914f35
to
40a65aa
Compare
40a65aa
to
077eca3
Compare
077eca3
to
eb2bafb
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.
Thanks for the initial review! I usually tend to use drafts for work-in-progress proposals which is pending my self-review. Here, it turned out not to be too far off from the final "ready for review" state ;)
// We try to remove the temporary cache dir after each test, but we don't | ||
// fail the test if it fails. For example, we have observed that in GHA | ||
// on Windows, the temp dir cannot be removed due to permission issues. | ||
try { | ||
await remove(cacheDir); | ||
} catch (error) { | ||
log("Failed to remove temporary cache dir", 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.
I added a try-catch and a comment about an issue I observed in https://github.com/NomicFoundation/hardhat/actions/runs/10332073748/job/28602920453, for example. Have you seen something similar before? Do you think it's worth ensuring we can always delete the temp dirs in CI? In this particular case I think it might have something to do that the temp dir is on C:
while the runner is set up to have perms and execute in the context of drive D:
.
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 actually sirprising, but I think we can live with it.
Maybe we should move it to the test utils package so that we don't hit this edge case again.
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 created an issue for this - #5602 If I cannot resolve it, I'm going to add the utility function for this.
// We accept ReplOptions as an argument to allow tests overriding the IO streams | ||
options?: repl.ReplOptions; |
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 really interesting! I never thought about it.
v-next/hardhat/src/internal/builtin-plugins/console/task-action.ts
Outdated
Show resolved
Hide resolved
v-next/hardhat/src/internal/builtin-plugins/console/task-action.ts
Outdated
Show resolved
Hide resolved
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 good! :)
I left a few comments, mostly about things that can be done later. Do you mind creating issues so that we don't loose track of them?
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 to add to Pato's comments. This looks great, nice work!
6ed7bbb
to
c23b974
Compare
Resolves #5575
This PR adds a
console
task to the next version ofhardhat
. Here's how it looks like:The task includes the following options:
history
- allows specifying the path where REPL history file should be stored. If it's relative, it will be resolved in the context of global hardhat cache dir. If it is explicitly set to an empty string, the history will not be preserved. It defaults toconsole-history.txt
in the global hardhat cache dir. This is very helpful during testing, but I thought it might be useful to the users, too.noCompile
- disables compilation task. This is not implemented yet.options
- This is a hidden argument. It is not exposed to the users. It is only used during testing to set custom REPL options. In particular, we override the IO streams during testing because otherwise the test runner hangs indefinitely.commands
- This is a variadic argument. One can provide a list of commands that should be executed in the REPL after the startup. It defaults to running the.help
command after startup. This is very helpful during testing because we want to run the commands after the entire REPL setup fully complete there, but I thought it might be useful to the users, too.Testing
example-project
;pnpm hardhat console