Add rename file action #46
Conversation
There was a problem hiding this comment.
Nice!
I tried it out to see what would happen if I asked for a change and a rename in the same file, and I end up not seeing the rename change at all? See result and log below. In this case what I think we should do is display "Rename x.py -> y.py" above the list of changes (if any) for that file (aka, that would replace the usual line showing which file is being changed above other changes).
|
The model hasn't given the rename file action by the time we've streamed the file name for the previous response; additionally, since the model's responses are applied in order, I think it's best to keep the change files the same as they are. I'm going to fix it to print the rename file action, but not change the other actions at all. |
PCSwingle
force-pushed
the
add-rename-file
branch
from
67632d3
to
240b1f6
Compare
ah...
within a file we sort changes, but you mean the order that we change files? |
|
I tried running this again and it still doesn't show me the rename? And then I applied all changes and it crashed after deleting the file (but not making a new one): 
|
PCSwingle
force-pushed
the
add-rename-file
branch
2 times, most recently
from
bc66ef6
to
91aa136
Compare
|
Just tried your prompt and it worked? Maybe it got fixed when I rebased on main (since there were a lot of path changes and things that I had to change) or I fixed it a few weeks ago and forgot. If you have the logs from that run still though I can see if I can reproduce it. |
Co-authored-by: biobootloader <[email protected]>
… add new test, fix bug with change and rename on same file
PCSwingle
force-pushed
the
add-rename-file
branch
from
91aa136
to
9145516
Compare
…de removing rename files, printing rename file changes, and add a test
|
@waydegg just finished fixing merge conflicts; would love to get your feedback on this PR before I merge it! |
| @@ -205,8 +205,16 @@ def user_filter_changes( | |||
| indices = [] | |||
| for index, change in enumerate(code_changes, start=1): | |||
| print_change(change) | |||
| # Allowing the user to remove rename file changes introduces a lot of edge cases | |||
There was a problem hiding this comment.
What edge cases have you discovered/thought about so far?
One that comes to mind is a file being renamed and imports not being updated accordingly.
There was a problem hiding this comment.
I was thinking more edge cases in how we handle it; the big one being that if we don't rename the file, then any changes the model suggests that try to change the renamed file will be trying to change a non-existent file. It gets worse if the model is renaming multiple files to similar names, or one file twice, and so on.
| @@ -86,7 +86,7 @@ def _get_files( | |||
| files = {} | |||
| for file in files_direct: | |||
| if file.path not in excluded_files | excluded_files_from_dir: | |||
| files[file.path] = file | |||
| files[Path(os.path.realpath(file.path))] = file | |||
There was a problem hiding this comment.
What's the thinking for using realpath here? Are there issues with using symbolic links?
There was a problem hiding this comment.
Or did you mean to use relpath instead?
There was a problem hiding this comment.
Well the real reason was because we were actually just putting a relative Path in there (when it was supposed to be an absolute path!); but yeah, the reason I use realpath instead of abspath is because of symlinks
| abs_path = self.config.git_root / delete_change.file | ||
| if not abs_path.exists(): | ||
| logging.error(f"Path {abs_path} non-existent on delete") | ||
| return |
There was a problem hiding this comment.
Instead of using self.config.git_root / delete_change.file we can use os.path.abspath(delete_change.file) to handle deletes for files outside of a repo. I know we already have this behavior in the main branch but I just noticed it lol. Pretty sure we're assuming all files are under a git repo across Mentat so it might be better to address this separately.
There was a problem hiding this comment.
This is actually necessary; abspath appends the CWD to the beginning of the path, which might not necessarily be the current git repo (it could be a subdirectory). This does remind me though, I don't think we have any tests where we run mentat from a subdiretory of the git repo it's actually in; we should probably think about adding some of those!
There was a problem hiding this comment.
I don't think we have any tests where we run mentat from a subdiretory of the git repo it's actually in; we should probably think about adding some of those!
@PCSwingle can you add an issue for this
No description provided.