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

Conversation

RajWorking
Copy link
Collaborator

Description

Deletion of code between specified range of lines is similar to insert. Moving code block is simply delete + insert calls.

How Has This Been Tested?

Unit tests and local testing.

Does this PR introduce a breaking change?

No.

@RajWorking RajWorking requested review from xingyaoww and ryanhoangt and removed request for xingyaoww November 16, 2024 18:23
Copy link
Collaborator

@ryanhoangt ryanhoangt left a comment

Choose a reason for hiding this comment

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

Regarding the lint issue, can you run make install-pre-commit-hooks to config pre-commit hook, and then try a dummy commit? I'll try to improve this and put in the README in future PRs

.gitignore Outdated
@@ -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.

@@ -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

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

"""
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.

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.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants