refactor(cli): Librarify nova_cli to avoid code duplication#916
refactor(cli): Librarify nova_cli to avoid code duplication#916aapoalas merged 1 commit intotrynova:mainfrom
nova_cli to avoid code duplication#916Conversation
niluxv
changed the title
nova_cli to avoid code duplicationnova_cli to avoid code duplication
niluxv
force-pushed
the
librarify-cli
branch
2 times, most recently
from
5bcfde9 to
0dbdc3a
Compare
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
This looks good to me: I don't unfortunately have any particularly good ideas about what a more generic API would look like. Storing parsed-but-not-executed Scripts could indeed be done using Global<Script>s, but I don't have a handle on if that would work nicely for a benchmarking CLI or not.
I'd be happy to merge this as-is; I kind of expect that figuring out the correct API is something that can only be done through experiment.
niluxv
changed the title
nova_cli to avoid code duplicationnova_cli to avoid code duplication
|
Ok, indeed we can further experiment after this is merged; the no semver guarantee gives us flexibility there. And it would be quite a nightmare to merge/rebase this if someone makes significant changes to the cli code in the meantime... How about I rebase this on top of main so we lose the merge commit, and squash my two commits together? |
Sounds good to me! |
Done. |
This already factored out some logic that was duplicated between the `eval` and `repl` subcommands. However, the real goal is to provide a library the benchmark runner can use, to avoid code duplication there.
aapoalas
force-pushed
the
librarify-cli
branch
from
3d97bb4 to
94752bf
Compare
|
@niluxv Sorry it took me so long to get back to you! I rebased the code again and will merge once the tests show the all green. Thank you so much for this, this is great! <3 |
3 participants
Right now this is mostly moving code around, so the benchmark runner (which I have in a separate branch) can share most of its code with the cli. I have no idea whether this library could be useful for users of nova as well, I don't really know their requirements, but I assume the current setup is not flexible enough for that. (For example, currently everything assumes there is only one realm.)
The benchmark runner needs to separately parse and execute scripts, while the cli can just immediately execute a parsed script, which is why I haven't touched the script parsing and execution logic of the cli. However, maybe we can store a list of parsed but not yet executed scripts in
HostDefined(alongside the module map of already executed modules, or maybe merge them?), and then implement convenience commands for script/module parsing and execution for theInstance. Or maybe that is overly complicated and it is better to leave this work to the library user.I would love to hear any suggestions, as I find myself really struggling to provide a flexible and convenient API.