-
Notifications
You must be signed in to change notification settings - Fork 237
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
Replacement parser inverse #172
Conversation
Inverse test refactors to prevent duplication. File edits are compared instead of llm messages.
b05f781
to
2f6679f
Compare
Impressively mentat almost wrote a correct replacement inverse on it's first try with the |
725a768
to
52a796a
Compare
Love this! I'll let @PCSwingle do a closer review, looks good though! |
mentat/parsers/replacement_parser.py
Outdated
@@ -119,3 +120,47 @@ def _add_code_block( | |||
) | |||
) | |||
return "" | |||
|
|||
def file_edits_to_llm_message( | |||
self, parsedLLMResponse: ParsedLLMResponse, git_root: Path | None = None |
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 should have mentioned this in the last PR as well, but why are we passing in the git_root here? Why not always just use GIT_ROOT.get()? With mock_git_root the context variable should always exist in tests.
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 originally added it because I wanted to pass it in the translation script but then I ended up mocking it there too. I'll remove it.
elif file_edit.is_deletion: | ||
action_indicator = "-" | ||
elif file_edit.rename_file_path is not None: | ||
action_indicator = file_edit.rename_file_path.as_posix() |
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 could be wrong, but isn't the rename_file_path also absolute? I think we also need to get it relative to the git root here too
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.
No actually. For both this parser and the block parser it ends up as a relative path. Maybe a bug in the parser if that's not what you intended.
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.
Hmm, that is a bug in the parser that could actually have some problems; I'll have to fix that (in a separate pr)
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.
Hmm, that is a bug in the parser that could actually have some problems; I'll have to fix that (in a separate pr)
can you make an issue for this @PCSwingle unless you are going ahead and doing it now
mentat/parsers/replacement_parser.py
Outdated
ans += f"@ {file_rel_path} {action_indicator}\n" | ||
|
||
for replacement in file_edit.replacements: | ||
if len(replacement.new_lines) == 0: |
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 could be simplified; just do the insert if starting == ending and len(new_lines) != 0 (although the second part shouldn't even be necessary, since if len(new_lines) is 0 and starting == ending then the replacement isn't doing anything) instead of having if else if else
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 is better. To be honest I was so happy with my test that when mentat's output worked I barely read it.
from mentat.utils import convert_string_to_asyncgen | ||
|
||
|
||
async def verify_inverse(parser): |
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 nice, thanks for making this!
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.
Looks great to me! Super impressive that mentat was able to do most of it on it's own too! One tiny comment at the end that I think would simplify the reverser a bit, but feel free to merge if you don't want to do that.
Inverse test refactors to prevent duplication. File edits are compared instead of llm messages.
Sorry I keep saying "X isn't worth doing right now" and the immediately doing it in the next PR.
attr
andpython
made is easier to compare the equality of theFileEdit
s than I realized. Strictly speaking we're still testing more than we care about because the test woudl fail if the inverse changed the order of theFileEdit
s. We could sort or add to a set before comparing but it seems hacky to have attr implement an order or hash method just for this test.