-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
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 don't know the code very well, but at a high level this looks fine.
I do wonder about all the stuff in test/input
, though. For example, do we really want to check in a copy of the source code of zip-a-folder
? Wouldn't it be better to check it out from its own repo on demand?
@@ -7,6 +7,7 @@ import { Prompt } from "../prompt/Prompt"; | |||
import { PromptSpec } from "../prompt/PromptSpec"; | |||
import { SourceLocation } from "../util/SourceLocation"; | |||
import { charAtPosition, nextPosition, prevPosition } from "../util/code-utils"; | |||
import { Completion } from "../prompt/Completion"; |
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.
Why do we need this import?
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 don't know the code very well, but at a high level this looks fine.
I do wonder about all the stuff in
test/input
, though. For example, do we really want to check in a copy of the source code ofzip-a-folder
? Wouldn't it be better to check it out from its own repo on demand?
This is probably better from a flakiness perspective (you don't want your tests to start to fail when the repo goes away or the commit you used is deleted, which does sometimes happen....). Agreed that including the project in the repo is a bit weird, though - the only better solution that I would suggest is creating a small testing project with a few tests just for this purpose, and including that in the repo.
Co-authored-by: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>
Co-authored-by: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>
I agree with your comments about not including zip-a-folder's source and that it's better to avoid the potential flakiness associated with downloading it as the tests run. I will work on replacing zip-a-folder with a small test project. Thanks! |
I rewrote the test to use a simple example project and removed the dependence on zip-a-folder. I also rewrote several other tests to avoid a dependency on other projects. Please let me know if you have any further comments. |
LGTM |
Adding a replay mode to LLMorpheus to make it possible to replay executions. Documentation has been updated and a test has been added.