Skip to content
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

Added functionality for moving code block and deleting code block. #13

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ ipython_config.py
# This is especially recommended for binary packages to ensure reproducibility, and is more
# commonly ignored for libraries.
# https://python-poetry.org/docs/basic-usage/#commit-your-poetrylock-file-to-version-control
#poetry.lock
poetry.lock
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add lock file into version control, similar to OH codebase, for consistency and reproducibility.


# pdm
# Similar to Pipfile.lock, it is generally recommended to include pdm.lock in version control.
Expand Down
152 changes: 120 additions & 32 deletions openhands_aci/editor/editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
'create',
'str_replace',
'insert',
'delete',
'undo_edit',
'move_code_block',
# 'jump_to_definition', TODO:
# 'find_references' TODO:
]
Expand All @@ -32,6 +34,7 @@ class OHEditor:
- create
- navigate
- edit files
- refactor code
The tool parameters are defined by Anthropic and are not editable.

Original implementation: https://github.com/anthropics/anthropic-quickstarts/blob/main/computer-use-demo/computer_use_demo/tools/edit.py
Expand All @@ -48,8 +51,9 @@ def __call__(
*,
command: Command,
path: str,
dst_path: str | None = None,
file_text: str | None = None,
view_range: list[int] | None = None,
lines_range: list[int] | None = None,
old_str: str | None = None,
new_str: str | None = None,
insert_line: int | None = None,
Expand All @@ -59,7 +63,7 @@ def __call__(
_path = Path(path)
self.validate_path(command, _path)
if command == 'view':
return self.view(_path, view_range)
return self.view(_path, lines_range)
elif command == 'create':
if not file_text:
raise
Expand All @@ -82,6 +86,20 @@ def __call__(
if not new_str:
raise EditorToolParameterMissingError(command, 'new_str')
return self.insert(_path, insert_line, new_str, enable_linting)
elif command == 'delete':
if not lines_range:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking should we raise error here or delete the whole file content

raise EditorToolParameterMissingError(command, 'lines_range')
return self.delete(_path, lines_range)
elif command == 'move_code_block':
if not lines_range:
raise EditorToolParameterMissingError(command, 'lines_range')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here

if insert_line is None:
raise EditorToolParameterMissingError(command, 'insert_line')
if not dst_path:
raise EditorToolParameterMissingError(command, 'dst_path')
_dst_path = Path(dst_path)
self.validate_path(command, _dst_path)
return self.move_code_block(_path, lines_range, _dst_path, insert_line)
elif command == 'undo_edit':
return self.undo_edit(_path)

Expand Down Expand Up @@ -165,43 +183,18 @@ def view(self, path: Path, view_range: list[int] | None = None) -> CLIResult:
return CLIResult(output=stdout, error=stderr)

file_content = self.read_file(path)
start_line = 1
file_content_lines = file_content.split('\n')

if not view_range:
start_line = 1
return CLIResult(
output=self._make_output(file_content, str(path), start_line)
)

if len(view_range) != 2 or not all(isinstance(i, int) for i in view_range):
raise EditorToolParameterInvalidError(
'view_range',
view_range,
'It should be a list of two integers.',
)

file_content_lines = file_content.split('\n')
num_lines = len(file_content_lines)
start_line, end_line = view_range
if start_line < 1 or start_line > num_lines:
raise EditorToolParameterInvalidError(
'view_range',
view_range,
f'Its first element `{start_line}` should be within the range of lines of the file: {[1, num_lines]}.',
)

if end_line > num_lines:
raise EditorToolParameterInvalidError(
'view_range',
view_range,
f'Its second element `{end_line}` should be smaller than the number of lines in the file: `{num_lines}`.',
)

if end_line != -1 and end_line < start_line:
raise EditorToolParameterInvalidError(
'view_range',
view_range,
f'Its second element `{end_line}` should be greater than or equal to the first element `{start_line}`.',
)
self._validate_range(view_range, num_lines)

start_line, end_line = view_range
if end_line == -1:
file_content = '\n'.join(file_content_lines[start_line - 1 :])
else:
Expand Down Expand Up @@ -275,6 +268,70 @@ def insert(
success_message += 'Review the changes and make sure they are as expected (correct indentation, no duplicate lines, etc). Edit the file again if necessary.'
return CLIResult(output=success_message)

def delete(self, path: Path, lines_range: list[int]) -> CLIResult:
"""
Deletes text content in file from the given range.
"""
try:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help remove all the try-catch surrounding self.read_file? I think it's maybe not required as we already catch them in the method itself.

file_text = self.read_file(path)
except Exception as e:
raise ToolError(f'Ran into {e} while trying to read {path}') from None

file_text = file_text.expandtabs()

file_text_lines = file_text.split('\n')
num_lines = len(file_text_lines)

self._validate_range(lines_range, num_lines)

start_line, end_line = lines_range # inclusive

new_file_text_lines = (
file_text_lines[:start_line] + file_text_lines[end_line + 1:]
)
snippet_lines = (
file_text_lines[max(0, start_line - SNIPPET_CONTEXT_WINDOW) : start_line]
+ file_text_lines[
end_line + 1 : min(num_lines, end_line + SNIPPET_CONTEXT_WINDOW)
]
)
new_file_text = '\n'.join(new_file_text_lines)
snippet = '\n'.join(snippet_lines)

self.write_file(path, new_file_text)
self._file_history[path].append(file_text)

success_message = f'The file {path} has been edited. '
success_message += self._make_output(
snippet,
'a snippet of the edited file',
# max(1, start_line - SNIPPET_CONTEXT_WINDOW + 1),
)

success_message += 'Review the changes and make sure they are as expected (correct indentation, no duplicate lines, etc). Edit the file again if necessary.'
return CLIResult(output=success_message)

def move_code_block(
self, from_file: Path, from_range: list[int], dst_file: Path, insert_line: int
) -> CLIResult:
"""
Move a block of code from one file to another.
"""
file_content = self.read_file(from_file)
file_content_lines = file_content.split('\n')
start_line, end_line = from_range
code_block = '\n'.join(
file_content_lines[start_line:]
if end_line == -1
else file_content_lines[start_line: end_line + 1]
)
delete_result = self.delete(from_file, from_range)
insert_result = self.insert(dst_file, insert_line, code_block, True)

return CLIResult(
output=f'Code block moved from {from_file} to {dst_file}.\n{delete_result.output}\n{insert_result.output}'
)

def validate_path(self, command: Command, path: Path) -> None:
Copy link
Collaborator

@ryanhoangt ryanhoangt Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do validation for the command to make sure path is not a directory? (And maybe a test case for it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are doing that above.

"""
Check that the path/command combination is valid.
Expand Down Expand Up @@ -307,6 +364,37 @@ def validate_path(self, command: Command, path: Path) -> None:
f'The path {path} is a directory and only the `view` command can be used on directories.',
)

def _validate_range(self, lines_range: list[int], num_lines):
if len(lines_range) != 2 or not all(isinstance(i, int) for i in lines_range):
raise EditorToolParameterInvalidError(
'lines_range',
lines_range,
'It should be a list of two integers.',
)

start_line, end_line = lines_range

if start_line < 1 or start_line > num_lines:
raise EditorToolParameterInvalidError(
'lines_range',
lines_range,
f'Its first element `{start_line}` should be within the range of lines of the file: {[1, num_lines]}.',
)

if end_line > num_lines:
raise EditorToolParameterInvalidError(
'lines_range',
lines_range,
f'Its second element `{end_line}` should be smaller than the number of lines in the file: `{num_lines}`.',
)

if end_line != -1 and end_line < start_line:
raise EditorToolParameterInvalidError(
'lines_range',
lines_range,
f'Its second element `{end_line}` should be greater than or equal to the first element `{start_line}`.',
)

def undo_edit(self, path: Path) -> CLIResult:
"""
Implement the undo_edit command.
Expand Down
43 changes: 43 additions & 0 deletions tests/integration/test_file_editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,49 @@ def test_insert_with_linting(editor):
)


def test_move_code_block(editor):
editor, test_file = editor
test_file.write_text(
'This is a test file.\nThis file is for testing purposes.\nfoo\nbar\nbaz'
)

second_test_file = test_file.parent / 'second_test.txt'
second_test_file.write_text(
'This is also a test file.\nSome text should be added above this.'
)

result = editor(
command='move_code_block',
path=str(test_file),
dst_path=str(second_test_file),
lines_range=[2, 3],
insert_line=1,
)
assert isinstance(result, CLIResult)
assert 'foo' not in test_file.read_text()
assert 'bar' not in test_file.read_text()
assert 'foo' in second_test_file.read_text()
assert 'bar' in second_test_file.read_text()
print(result.output)
assert (
result.output
== f"""Code block moved from {test_file} to {second_test_file}.
The file {test_file} has been edited. Here's the result of running `cat -n` on a snippet of the edited file:
1\tThis is a test file.
2\tThis file is for testing purposes.
3\tbaz
Review the changes and make sure they are as expected (correct indentation, no duplicate lines, etc). Edit the file again if necessary.
The file {second_test_file} has been edited. Here's the result of running `cat -n` on a snippet of the edited file:
1\tThis is also a test file.
2\tfoo
3\tbar
4\tSome text should be added above this.

No linting issues found in the changes.
Review the changes and make sure they are as expected (correct indentation, no duplicate lines, etc). Edit the file again if necessary."""
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case for delete as well?


def test_insert_invalid_line(editor):
editor, test_file = editor
with pytest.raises(EditorToolParameterInvalidError) as exc_info:
Expand Down
Loading