feat(workon): refine a given file

skyl commented 1 week ago

PR Type

Enhancement, Tests

Description

Integrated a new workon feature into the corpora API and CLI, allowing for file editing and interaction.
Added workon_router to the API routing and implemented the corresponding CLI command.
Introduced new models and schemas for handling file chat operations.
Enhanced the documentation with new purpose, structure, and voice guidelines for the Corpora project.
Fixed bugs related to file deletion logic in the corpus router and CLI commands.
Added unit tests for the new workon functionality and updated existing tests to include workon_api.
Updated shell setup script with new aliases for improved developer experience.

Changes walkthrough 📝

Relevant files

Enhancement

11 files

router.py `Integrate workon_router into corpora API routing` py/packages/corpora/router.py Added import for `workon_router`. Added `workon_router` to the API router.	+3/-0
plan.py `Update message handling in plan router` py/packages/corpora/routers/plan.py - Modified message concatenation logic for split context.	+1/-1
workon.py `Add workon_router for file editing in corpora` py/packages/corpora/routers/workon.py Implemented new `workon_router` with file editing functionality. Added schemas for message and file chat.	+115/-0
workon.py `Implement workon CLI command for file interaction` py/packages/corpora_cli/commands/workon.py Added new CLI command for working on files. Implemented REPL loop for file revision.	+98/-0
context.py `Extend context object with workon_api` py/packages/corpora_cli/context.py - Added `workon_api` to context object.	+4/-2
main.py `Integrate workon command into CLI and API clients` py/packages/corpora_cli/main.py Integrated `workon` command into CLI. Updated API client initialization.	+10/-3
__init__.py `Add WorkonApi to corpora client package` py/packages/corpora_client/__init__.py - Imported `WorkonApi` and related models.	+3/-1
__init__.py `Include WorkonApi in API package` py/packages/corpora_client/api/__init__.py - Imported `WorkonApi`.	+1/-0
workon_api.py `Implement WorkonApi for file operations` py/packages/corpora_client/api/workon_api.py - Added new `WorkonApi` class with file method.	+286/-0
corpus_file_chat_schema.py `Define CorpusFileChatSchema model` py/packages/corpora_client/models/corpus_file_chat_schema.py - Added `CorpusFileChatSchema` model.	+123/-0
corpus_update_files_schema.py `Rename UpdateCorpusSchema to CorpusUpdateFilesSchema` py/packages/corpora_client/models/corpus_update_files_schema.py - Renamed `UpdateCorpusSchema` to `CorpusUpdateFilesSchema`.	+4/-4

Bug fix

2 files

corpus.py `Fix file deletion logic in corpus router` py/packages/corpora/routers/corpus.py Commented out debug print statements. Fixed bug with file deletion by splitting file names.	+5/-2
corpus.py `Refactor file deletion logic in corpus CLI command` py/packages/corpora_cli/commands/corpus.py Refactored file deletion logic. Added comments for potential bug in ninja.	+7/-1

Tests

3 files

test_main.py `Add tests for workon_api integration in CLI` py/packages/corpora_cli/test_main.py - Added tests for `workon_api` in CLI context.	+4/-2
test_corpus_file_chat_schema.py `Add unit tests for CorpusFileChatSchema` py/packages/corpora_client/test/test_corpus_file_chat_schema.py - Added unit tests for `CorpusFileChatSchema`.	+70/-0
test_workon_api.py `Add unit test stubs for WorkonApi` py/packages/corpora_client/test/test_workon_api.py - Added unit test stubs for `WorkonApi`.	+38/-0

Configuration changes

1 files

setup.sh

Update shell aliases in setup script

.devcontainer/setup.sh

Updated alias for tree command.

Added alias for deleting git branches.

+2/-1

Documentation

6 files

PURPOSE.md `Add purpose description for Corpora project` .corpora/PURPOSE.md - Added purpose description for Corpora project.	+1/-0
STRUCTURE.md `Document structure of Corpora repository` .corpora/STRUCTURE.md - Added structure description for Corpora repository.	+23/-0
VOICE.md `Define voice guidelines for documentation` .corpora/VOICE.md - Added voice guidelines for Corpora documentation.	+1/-0
DIRECTIONS.md `Provide directions for markdown files` .corpora/md/DIRECTIONS.md - Added directions for markdown files.	+1/-0
DIRECTIONS.md `Provide directions for Python development` .corpora/py/DIRECTIONS.md - Added directions for Python development.	+15/-0
README.md `Update README with project details and contribution guide` README.md Expanded project description and key components. Added contributing guidelines.	+17/-5

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

github-actions[bot] commented 1 week ago

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/skyl/corpora/commit/11fb456a77230063bf800d75f5e5b4420ee05221)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Possible Bug
The new code attempts to split `update.delete_files[0]` by commas, which assumes that the first element is a string containing multiple file paths. This may lead to unexpected behavior if `update.delete_files` is not structured as expected. Code Smell
The function `get_additional_context` uses multiple `if` statements to append context. Consider refactoring to make it more concise and maintainable. Code Smell
The REPL loop in the `file` function could be refactored for better readability and maintainability. Consider extracting parts of the loop into separate functions.

github-actions[bot] commented 1 week ago

PR Code Suggestions ✨

Latest suggestions up to 11fb456 Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Implement error handling for file reading to manage non-existent files gracefully ___ Handle the case where the file at `path` might not exist to avoid a `FileNotFoundError`. [py/packages/corpora_cli/commands/workon.py [26-28]](https://github.com/skyl/corpora/pull/30/files#diff-74cbdbf557fb8f2384cbdef97cc6b4ec6145b31ba29ba5365a80f04610a4abfcR26-R28) ```diff -with open(path, "r") as f: - current_file_content = f.read() if f else "" +try: + with open(path, "r") as f: + current_file_content = f.read() +except FileNotFoundError: + current_file_content = "" ``` Suggestion importance[1-10]: 9 Why: This suggestion effectively handles potential `FileNotFoundError` by implementing a try-except block, ensuring the program continues to run smoothly even if the file does not exist.	9
Possible bug	Validate the existence and type of `update.delete_files[0]` before splitting it ___ Ensure that `update.delete_files[0]` is a valid string before calling `split(",")` to avoid potential index errors or unexpected behavior. [py/packages/corpora/routers/corpus.py [78]](https://github.com/skyl/corpora/pull/30/files#diff-3ac4611171c3ba2b1e471a17f85e949a5df6e835391d39367db05286216d1762R78-R78) ```diff -delete_files = update.delete_files[0].split(",") +delete_files = update.delete_files[0].split(",") if update.delete_files and isinstance(update.delete_files[0], str) else [] ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential bug by ensuring that `update.delete_files[0]` is a valid string before attempting to split it. This prevents index errors or unexpected behavior, enhancing the robustness of the code.	8
Possible issue	Add a check to ensure `payload.messages` has at least two elements before accessing them ___ Consider handling cases where `payload.messages` might have fewer than two elements to prevent potential index errors. [py/packages/corpora/routers/plan.py [48]](https://github.com/skyl/corpora/pull/30/files#diff-d17ee5521ab86acd2823afbc993ced9ea8afb2f074db451ff29c56b83f4e33aaR48-R48) ```diff -"\n".join(message for message in payload.messages[-2:]) +"\n".join(message for message in payload.messages[-2:] if len(payload.messages) >= 2) ``` Suggestion importance[1-10]: 7 Why: The suggestion improves the code by handling cases where `payload.messages` might have fewer than two elements, preventing potential index errors. This enhances the code's reliability.	7
Enhancement	Remove `vi_mode=True` from the prompt to prevent user confusion unless vi keybindings are necessary ___ Avoid using `vi_mode=True` in `session.prompt` if users are not expected to be familiar with vi keybindings, as it might confuse them. [py/packages/corpora_cli/commands/workon.py [44-48]](https://github.com/skyl/corpora/pull/30/files#diff-74cbdbf557fb8f2384cbdef97cc6b4ec6145b31ba29ba5365a80f04610a4abfcR44-R48) ```diff user_input = session.prompt( ("What to do?\n" if not messages else "How to revise?\n"), multiline=True, - vi_mode=True, ) ``` Suggestion importance[1-10]: 5 Why: The suggestion improves user experience by removing `vi_mode=True`, which might confuse users unfamiliar with vi keybindings. However, it is more of a usability enhancement than a critical fix.	5

Previous suggestions

Suggestions up to commit 4dac84a

Category	Suggestion	Score
Possible bug	Import the `os` module to prevent a potential `NameError` in the example code ___ Ensure that the `os` module is imported at the beginning of the example code to avoid a potential `NameError` when accessing `os.environ["BEARER_TOKEN"]`. [py/packages/corpora_client/docs/WorkonApi.md [38]](https://github.com/skyl/corpora/pull/30/files#diff-5ee80e423089ce094ba455c1331e9f718ccb1fb76dea7f81007b6f69079bd3f6R38-R38) ```diff +import os +... access_token = os.environ["BEARER_TOKEN"] ``` Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a potential `NameError` due to the missing import of the `os` module, which is necessary for accessing environment variables. This is a critical fix for the example code to function properly.	9
Possible bug	Add a check to ensure the list has enough elements before accessing the last two items ___ Ensure that the `payload.messages` list contains at least two elements before attempting to access the last two messages to prevent potential index errors. [py/packages/corpora/routers/plan.py [48]](https://github.com/skyl/corpora/pull/30/files#diff-d17ee5521ab86acd2823afbc993ced9ea8afb2f074db451ff29c56b83f4e33aaR48-R48) ```diff -"\n".join(message for message in payload.messages[-2:]) +"\n".join(message for message in payload.messages[-2:]) if len(payload.messages) >= 2 else "" ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential index error by ensuring that the list has at least two elements before accessing the last two. This is crucial for preventing runtime errors and improving code robustness.	8
Best practice	Add exception handling for file reading operations to manage potential file access errors ___ Consider handling exceptions that may occur during file reading operations to prevent the application from crashing if a file is not found or cannot be read. [py/packages/corpora/routers/workon.py [28-29]](https://github.com/skyl/corpora/pull/30/files#diff-ebdc3ac7a2220924713b8ee22440e52ccc30c49a601ae08f2e345bdeb196fb43R28-R29) ```diff -with open(path, "r") as f: - current_file_content = f.read() if f else "" +try: + with open(path, "r") as f: + current_file_content = f.read() if f else "" +except FileNotFoundError: + current_file_content = "" ``` Suggestion importance[1-10]: 7 Why: Adding exception handling for file reading operations is a good practice to prevent the application from crashing due to file access errors, enhancing the application's robustness.	7
Possible issue	Ensure the `revision` is not empty before writing to a file ___ Add a check to ensure that the `revision` variable is not empty before attempting to write it to a file to prevent writing empty content. [py/packages/corpora_cli/commands/workon.py [85-87]](https://github.com/skyl/corpora/pull/30/files#diff-74cbdbf557fb8f2384cbdef97cc6b4ec6145b31ba29ba5365a80f04610a4abfcR85-R87) ```diff -if typer.confirm("Write file?"): +if revision and typer.confirm("Write file?"): with open(path, "w") as f: f.write(revision) ``` Suggestion importance[1-10]: 7 Why: This suggestion prevents writing empty content to a file, which could lead to data loss or corruption. It adds an additional layer of validation, enhancing the reliability of the file writing process.	7
Maintainability	Rename the `PromptSession` instance to avoid conflicts with the imported `session` ___ Avoid overwriting the `session` variable by renaming the `PromptSession` instance to prevent potential conflicts with the imported `session` from `requests`. [py/packages/corpora_cli/commands/workon.py [14]](https://github.com/skyl/corpora/pull/30/files#diff-74cbdbf557fb8f2384cbdef97cc6b4ec6145b31ba29ba5365a80f04610a4abfcR14-R14) ```diff -session = PromptSession() +prompt_session = PromptSession() ``` Suggestion importance[1-10]: 6 Why: Renaming the `PromptSession` instance helps avoid potential conflicts with the imported `session` from `requests`, improving code maintainability and preventing unexpected behavior.	6