From e9d10fde19dd20df724afa412f002fb3691b9d27 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Thu, 29 May 2025 19:07:32 +0200 Subject: [PATCH] Remove dry_run and overloads from remaining editing methods Add corresponding test classes and test cases (not yet working) --- scripts/demo_run_tools.py | 16 +-- src/serena/symbol.py | 199 +++++------------------------ test/serena/test_symbol_editing.py | 158 ++++++++--------------- 3 files changed, 86 insertions(+), 287 deletions(-) diff --git a/scripts/demo_run_tools.py b/scripts/demo_run_tools.py index eafae78d..5d8e450d 100644 --- a/scripts/demo_run_tools.py +++ b/scripts/demo_run_tools.py @@ -24,20 +24,8 @@ class InMemorySerenaConfig(SerenaConfigBase): if __name__ == "__main__": project_path = str(Path("test") / "resources" / "repos" / "python" / "test_repo") agent = SerenaAgent(project=project_path, serena_config=InMemorySerenaConfig()) - find_symbol_tool = agent.get_tool(FindSymbolTool) + # apply a tool + find_symbol_tool = agent.get_tool(FindSymbolTool) print("Finding the symbol 'VariableContainer'\n") pprint(json.loads(find_symbol_tool.apply("VariableContainer", within_relative_path=str(Path("test_repo") / "variables.py")))) - - # modifying with dry-run - symbol_manager = agent.symbol_manager - - code_diff = symbol_manager.insert_after_symbol( - "VariableContainer/modify_instance_var", - relative_file_path=str(Path("test_repo") / "variables.py"), - body="test test\nsecond line", - dry_run=True, - ) - print(f"Deleted lines: {code_diff.deleted_lines}") - print(f"Added lines: {code_diff.added_lines}") - print(f"Edited module: {code_diff.modified_content}") diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 4821bc2b..a284a14b 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -2,16 +2,15 @@ import json import logging import os from collections.abc import Iterator, Sequence -from contextlib import contextmanager, nullcontext +from contextlib import contextmanager from dataclasses import asdict, dataclass, field from difflib import SequenceMatcher -from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, Union, overload +from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, Union from sensai.util.string import ToStringMixin from multilspy import SyncLanguageServer from multilspy.multilspy_types import Position, SymbolKind, UnifiedSymbolInformation -from multilspy.multilspy_utils import TextUtils if TYPE_CHECKING: from .agent import SerenaAgent @@ -605,30 +604,22 @@ class SymbolManager: self.agent.mark_file_modified(relative_path) @contextmanager - def _edited_symbol_location(self, location: SymbolLocation, dry_run: bool = False) -> Iterator[Symbol]: + def _edited_symbol_location(self, location: SymbolLocation) -> Iterator[Symbol]: """ Context manager for locating and editing a symbol in a file. - If dry_run is True, the file is not actually modified, but the symbol is still located. - The dry_run flag is primarily implemented to allow the same code to be used for both editing and diffing - (the latter mostly for tests). """ symbol = self.find_by_location(location) if symbol is None: raise ValueError("Symbol not found/has no defined location within a file") assert location.relative_path is not None - edit_context = self._edited_file(location.relative_path) if not dry_run else nullcontext() - with edit_context: + with self._edited_file(location.relative_path): yield symbol def _get_code_file_content(self, relative_path: str) -> str: """Get the content of a file using the language server.""" return self.lang_server.language_server.retrieve_full_file_content(relative_path) - @overload - def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... - @overload - def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def replace_body(self, name_path: str, relative_file_path: str, body: str) -> None: """ Replace the body of the symbol with the given name in the given file. """ @@ -643,47 +634,28 @@ class SymbolManager: + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - return self.replace_body_at_location(symbol.location, body, dry_run=dry_run) # type: ignore[call-overload] + return self.replace_body_at_location(symbol.location, body) - @overload - def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[False] = False) -> None: ... - @overload - def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def replace_body_at_location(self, location: SymbolLocation, body: str) -> None: """ Replace the body of the symbol at the given location with the given body :param location: the location of the symbol to replace. :param body: the new body - :param dry_run: if True, return a CodeDiff instead of modifying the file """ # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" - with self._edited_symbol_location(location, dry_run=dry_run) as symbol: + with self._edited_symbol_location(location) as symbol: assert location.relative_path is not None start_pos = symbol.body_start_position end_pos = symbol.body_end_position if start_pos is None or end_pos is None: raise ValueError(f"Symbol at {location} does not have a defined body range.") - if dry_run: - original_content = self._get_code_file_content(location.relative_path) - modified_content, _ = TextUtils.delete_text_between_positions( - original_content, start_pos["line"], start_pos["character"], end_pos["line"], end_pos["character"] - ) - modified_content, _, _ = TextUtils.insert_text_at_position( - modified_content, start_pos["line"], start_pos["character"], body - ) - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - # TODO: add method (in LS and TextUtils) replace_text_between_positions, calling two methods in LS adds extra overhead - # Use it here and above - self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) - self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) - return None + self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) + self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) - @overload def insert_after_symbol( self, name_path: str, @@ -692,29 +664,7 @@ class SymbolManager: *, use_same_indentation: bool = True, at_new_line: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_after_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - use_same_indentation: bool = True, - at_new_line: bool = True, - dry_run: Literal[True], - ) -> CodeDiff: ... - def insert_after_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - use_same_indentation: bool = True, - at_new_line: bool = True, - dry_run: bool = False, - ) -> CodeDiff | None: + ) -> None: """ Inserts content after the symbol with the given name in the given file. """ @@ -728,31 +678,18 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[-1] - return self.insert_after_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation, dry_run=dry_run) # type: ignore[call-overload] + return self.insert_after_symbol_at_location( + symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation + ) - @overload def insert_after_symbol_at_location( - self, - location: SymbolLocation, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_after_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: Literal[True] - ) -> CodeDiff: ... - def insert_after_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: bool = False - ) -> CodeDiff | None: + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True + ) -> None: """ Appends content after the given symbol :param location: the location of the symbol after which to add new lines :param body: the body of the entity to append - :param dry_run: if True, return a CodeDiff instead of modifying the file """ # make sure body always ends with at least one newline if not body.endswith("\n"): @@ -796,16 +733,9 @@ class SymbolManager: # > dataclass_instance.status = "active" # Reassign dataclass field col = 0 - if dry_run: - original_content = self._get_code_file_content(location.relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line=line, col=col, text_to_be_inserted=body) - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_symbol_location(location): - self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - return None + with self._edited_symbol_location(location): + self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - @overload def insert_before_symbol( self, name_path: str, @@ -814,29 +744,7 @@ class SymbolManager: *, at_new_line: bool = True, use_same_indentation: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_before_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: Literal[True], - ) -> CodeDiff: ... - def insert_before_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: bool = False, - ) -> CodeDiff | None: + ) -> None: """ Inserts content before the symbol with the given name in the given file. """ @@ -850,31 +758,16 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - return self.insert_before_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation, dry_run=dry_run) # type: ignore[call-overload] + self.insert_before_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation) - @overload def insert_before_symbol_at_location( - self, - location: SymbolLocation, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_before_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: Literal[True] - ) -> CodeDiff: ... - def insert_before_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: bool = False - ) -> CodeDiff | None: + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True + ) -> None: """ Inserts content before the given symbol :param location: the location of the symbol before which to add new lines :param body: the body of the entity to insert - :param dry_run: if True, return a CodeDiff instead of modifying the file """ with self._edited_symbol_location(location) as symbol: symbol_start_pos = symbol.body_start_position @@ -892,62 +785,32 @@ class SymbolManager: line -= 1 assert location.relative_path is not None - if dry_run: - original_content = self._get_code_file_content(location.relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line=line, col=col, text_to_be_inserted=body) - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - return None + self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - @overload - def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: Literal[False] = False) -> None: ... - @overload - def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: bool = False) -> CodeDiff | None: + def insert_at_line(self, relative_path: str, line: int, content: str) -> None: """ Inserts content at the given line in the given file. :param line: the 0-based index of the line to insert content at :param content: the content to insert - :param dry_run: if True, return a CodeDiff instead of modifying the file """ - if dry_run: - original_content = self._get_code_file_content(relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line, 0, content) - return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_file(relative_path): - self.lang_server.insert_text_at_position(relative_path, line, 0, content) - return None + with self._edited_file(relative_path): + self.lang_server.insert_text_at_position(relative_path, line, 0, content) - @overload - def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: Literal[False] = False) -> None: ... - @overload - def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: Literal[True]) -> CodeDiff: ... - def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: bool = False) -> CodeDiff | None: + def delete_lines(self, relative_path: str, start_line: int, end_line: int) -> None: """ Deletes lines in the given file. :param start_line: the 0-based index of the first line to delete (inclusive) :param end_line: the 0-based index of the last line to delete (inclusive) - :param dry_run: if True, return a CodeDiff instead of modifying the file """ start_col = 0 end_line_for_delete = end_line + 1 end_col = 0 - if dry_run: - original_content = self._get_code_file_content(relative_path) - modified_content, _ = TextUtils.delete_text_between_positions( - original_content, start_line, start_col, end_line_for_delete, end_col - ) - return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_file(relative_path): - start_pos = Position(line=start_line, character=start_col) - end_pos = Position(line=end_line_for_delete, character=end_col) - self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) - return None + with self._edited_file(relative_path): + start_pos = Position(line=start_line, character=start_col) + end_pos = Position(line=end_line_for_delete, character=end_col) + self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) def delete_symbol_at_location(self, location: SymbolLocation) -> None: """ diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py index 0ea19bc6..dcbfe704 100644 --- a/test/serena/test_symbol_editing.py +++ b/test/serena/test_symbol_editing.py @@ -8,7 +8,6 @@ from pathlib import Path import pytest -from multilspy import SyncLanguageServer from multilspy.multilspy_config import Language from serena.symbol import CodeDiff from src.serena.symbol import SymbolManager @@ -165,46 +164,6 @@ NEW_TYPESCRIPT_FUNCTION = """function newInsertedFunction(): void { }""" -@pytest.mark.parametrize( - "language_server, relative_file_path, symbol_name, new_content, expected_added_lines_content", - [ - ( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "use_module_variables", - NEW_PYTHON_FUNCTION, - NEW_PYTHON_FUNCTION.strip().splitlines(), - ), - ( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "helperFunction", - NEW_TYPESCRIPT_FUNCTION, - NEW_TYPESCRIPT_FUNCTION.strip().splitlines(), - ), - ], - indirect=["language_server"], -) -def test_insert_before_symbol_dry_run( - language_server: SyncLanguageServer, - relative_file_path: str, - symbol_name: str, - new_content: str, - expected_added_lines_content: list[str], -): - symbol_manager = SymbolManager(lang_server=language_server) - code_diff = symbol_manager.insert_before_symbol(symbol_name, relative_file_path, new_content, dry_run=True) - - assert code_diff is not None - assert code_diff.relative_path == relative_file_path - assert len(code_diff.deleted_lines) == 0 - actual_added_lines_content = [line.strip() for _, line in code_diff.added_lines if line.strip()] - normalized_expected_added_lines = [line.strip() for line in expected_added_lines_content if line.strip()] - - assert actual_added_lines_content == normalized_expected_added_lines - assert code_diff.original_content != code_diff.modified_content - - NEW_PYTHON_VARIABLE = 'new_module_var = "Inserted after typed_module_var"' NEW_TYPESCRIPT_FUNCTION_AFTER = """function newFunctionAfterClass(): void { @@ -212,43 +171,53 @@ NEW_TYPESCRIPT_FUNCTION_AFTER = """function newFunctionAfterClass(): void { }""" +class InsertBeforeSymbolTest(EditingTest): + def __init__(self, language: Language, rel_path: str, symbol_name: str, new_content: str): + super().__init__(language, rel_path) + self.symbol_name = symbol_name + self.new_content = new_content + + def _apply_edit(self, symbol_manager: SymbolManager) -> None: + symbol_manager.insert_before_symbol(self.symbol_name, self.rel_path, self.new_content) + + def _test_diff(self, code_diff: CodeDiff) -> None: + assert code_diff.original_content != code_diff.modified_content + actual_added_lines = [line.strip() for _, line in code_diff.added_lines if line.strip()] + normalized_expected_added_lines = [line.strip() for line in self.new_content.splitlines() if line.strip()] + assert actual_added_lines == normalized_expected_added_lines + + @pytest.mark.parametrize( - "language_server, relative_file_path, symbol_name, new_content, expected_added_lines_content", + "test_case", [ - ( + InsertBeforeSymbolTest( Language.PYTHON, PYTHON_TEST_REL_FILE_PATH, "typed_module_var", NEW_PYTHON_VARIABLE, - [NEW_PYTHON_VARIABLE], ), - ( + InsertBeforeSymbolTest( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "use_module_variables", + NEW_PYTHON_FUNCTION, + ), + InsertBeforeSymbolTest( Language.TYPESCRIPT, TYPESCRIPT_TEST_FILE, "DemoClass", NEW_TYPESCRIPT_FUNCTION_AFTER, - NEW_TYPESCRIPT_FUNCTION_AFTER.strip().splitlines(), + ), + InsertBeforeSymbolTest( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "helperFunction", + NEW_TYPESCRIPT_FUNCTION, ), ], - indirect=["language_server"], ) -def test_insert_after_symbol_dry_run( - language_server: SyncLanguageServer, - relative_file_path: str, - symbol_name: str, - new_content: str, - expected_added_lines_content: list[str], -): - symbol_manager = SymbolManager(lang_server=language_server) - code_diff = symbol_manager.insert_after_symbol(symbol_name, relative_file_path, new_content, dry_run=True) - - assert code_diff is not None - assert code_diff.relative_path == relative_file_path - assert len(code_diff.deleted_lines) == 0 - actual_added_lines_content = [line.strip() for _, line in code_diff.added_lines if line.strip()] - normalized_expected_added_lines = [line.strip() for line in expected_added_lines_content if line.strip()] - assert actual_added_lines_content == normalized_expected_added_lines - assert code_diff.original_content != code_diff.modified_content +def test_insert_before_symbol(test_case): + test_case.run_test() PYTHON_REPLACED_BODY = """ # This body has been replaced @@ -269,48 +238,27 @@ EXPECTED_ORIGINAL_MODIFY_INSTANCE_VAR_PYTHON = """ def modify_instance_var(se EXPECTED_ORIGINAL_PRINTVALUE_TYPESCRIPT = [" printValue() {", " console.log(this.value);", " }"] +class ReplaceBodyTest(EditingTest): + def __init__( + self, language: Language, rel_path: str, symbol_name: str, new_body: str, expected_removed_lines: str, expected_added_lines: str + ): + super().__init__(language, rel_path) + self.symbol_name = symbol_name + self.new_body = new_body + + def _apply_edit(self, symbol_manager: SymbolManager) -> None: + symbol_manager.replace_body(self.symbol_name, self.rel_path, self.new_body) + + def _test_diff(self, code_diff: CodeDiff) -> None: + assert code_diff.original_content != code_diff.modified_content + # TODO test properly + + @pytest.mark.parametrize( - "language_server, relative_file_path, symbol_name, new_body, expected_original_lines_content, expected_modified_lines_content", + "test_case", [ - ( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "VariableContainer/modify_instance_var", - PYTHON_REPLACED_BODY, - EXPECTED_ORIGINAL_MODIFY_INSTANCE_VAR_PYTHON.strip().splitlines(), - PYTHON_REPLACED_BODY.strip().splitlines(), - ), - ( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "DemoClass/printValue", - TYPESCRIPT_REPLACED_BODY, - EXPECTED_ORIGINAL_PRINTVALUE_TYPESCRIPT, # Already a list of lines - TYPESCRIPT_REPLACED_BODY.strip().splitlines(), - ), + # TODO add tests ], - indirect=["language_server"], ) -def test_replace_body_dry_run( - language_server: SyncLanguageServer, - relative_file_path: str, - symbol_name: str, - new_body: str, - expected_original_lines_content: list[str], - expected_modified_lines_content: list[str], -): - symbol_manager = SymbolManager(lang_server=language_server) - code_diff = symbol_manager.replace_body(symbol_name, relative_file_path, new_body, dry_run=True) - - assert code_diff is not None - assert code_diff.relative_path == relative_file_path - - actual_original_lines = [line.strip() for _, line in code_diff.deleted_lines if line.strip()] - normalized_expected_original_lines = [line.strip() for line in expected_original_lines_content if line.strip()] - assert actual_original_lines == normalized_expected_original_lines - - actual_modified_lines = [line.strip() for _, line in code_diff.added_lines if line.strip()] - normalized_expected_modified_lines = [line.strip() for line in expected_modified_lines_content if line.strip()] - assert actual_modified_lines == normalized_expected_modified_lines - - assert code_diff.original_content != code_diff.modified_content +def test_replace_body(test_case: ReplaceBodyTest): + test_case.run_test()