Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
samuelcolvin
left a comment
samuelcolvin
left a comment
There was a problem hiding this comment.
overall this looks good, let's not bloat run.rs too much - the new stuff can go into a new repl.rs.
@davidhewitt should review this too.
|
Also required (could be done in this PR or a followup):
|
|
Thanks! I am on the edge splitting run.rs to two modules, do you want me to replicate similar structure (ie executors and all kind of plumbing) in separate file? it kind of makes sense since it is a different execution model, but it will produce mostly duplicated code, is it ok? |
|
Updated:
|
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
This is a huge PR, I will probably review in multiple rounds. I have some initial questions which might affect the content here, so let's address those first before I review further.
crates/monty-cli/src/main.rs
Outdated
| fn main() -> ExitCode { | ||
| let args: Vec<String> = env::args().collect(); | ||
| let file_path = if args.len() > 1 { &args[1] } else { "example.py" }; | ||
| let repl_mode = matches!(args.get(1).map(String::as_str), Some("--repl" | "-r")); |
There was a problem hiding this comment.
This feels brittle, I would recommend we convert argument parsing to clap. (Might be better to do that as a separate precursor PR.)
|
Resolved all comments, except arg parsing, but it is robust. |
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks - some follow up questions / thoughts. This is a big PR so please forgive if it takes me multiple reviews to fully digest.
|
Changed it, to use "-i" instead, also multiline support, i am not 100% sure if this is a good way to parse code and see if block is completed, aren't this too flimsy? on other side it is CLI probably wont be used in production anyway |
3 participants
Hello!
I will be straight - it was generated by codex, i reviewed it and seemed fine - it clones compiled functions, internal states but keeps heap untouched.
This is a pretty much a must for RLM implementations which monty is perfect for.