LocalRepoService fixes #266
Conversation
WalkthroughThe pull request introduces a new method Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant L as LocalRepoService
participant G as _get_contents Method
C->>L: Request repository structure
L->>G: Invoke _get_contents(path)
G-->>L: Return structured contents (files/directories)
L-->>C: Deliver repository structure or error
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/modules/code_provider/local_repo/local_repo_service.py (1)
61-61:⚠️ Potential issueFix comparison to None.
The pipeline warning indicates that comparison to
Noneshould useisoperator.Apply this diff to fix the warning:
- if (start_line == end_line == 0) or (start_line == end_line == None): + if (start_line == end_line == 0) or (start_line is None and end_line is None):🧰 Tools
🪛 Ruff (0.8.2)
61-61: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
🪛 GitHub Actions: Pre-commit
[warning] 61-61: Comparison to
Noneshould becond is None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/modules/code_provider/local_repo/local_repo_service.py(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pre-commit
app/modules/code_provider/local_repo/local_repo_service.py
[warning] 61-61: Comparison to None should be cond is None
🔇 Additional comments (2)
app/modules/code_provider/local_repo/local_repo_service.py (2)
6-6: LGTM! Good addition of base_path parameter.The changes properly handle path initialization and provide flexibility in repository location.
Also applies to: 18-18, 25-25
156-156: LGTM! Clean adaptation to the new content structure.The changes properly handle the new dictionary-based content structure while maintaining the existing filtering logic.
Also applies to: 164-168, 173-173, 176-176, 179-179, 188-189
| def _get_contents(self, path: str) -> Union[List[dict], dict]: | ||
| """ | ||
| If the path is a directory, it returns a list of dictionaries, | ||
| each representing a file or subdirectory. If the path is a file, its content is read and returned. | ||
|
|
||
| :param path: Relative or absolute path within the local repository. | ||
| :return: A dict if the path is a file (with file content loaded), or a list of dicts if the path is a directory. | ||
| """ | ||
| if not isinstance(path, str): | ||
| raise TypeError(f"Expected path to be a string, got {type(path).__name__}") | ||
|
|
||
| if path == "/": | ||
| path = "" | ||
|
|
||
| abs_path = os.path.abspath(path) | ||
|
|
||
| if not os.path.exists(abs_path): | ||
| raise FileNotFoundError(f"Path '{abs_path}' does not exist.") | ||
|
|
||
| if os.path.isdir(abs_path): | ||
| contents = [] | ||
| for item in os.listdir(abs_path): | ||
| item_path = os.path.join(abs_path, item) | ||
| if os.path.isdir(item_path): | ||
| contents.append({ | ||
| "path": item_path, | ||
| "name": item, | ||
| "type": "dir", | ||
| "content": None, #path is a dir, content is not loaded | ||
| "completed": True | ||
| }) | ||
| elif os.path.isfile(item_path): | ||
| contents.append({ | ||
| "path": item_path, | ||
| "name": item, | ||
| "type": "file", | ||
| "content": None, | ||
| "completed": False | ||
| }) | ||
| else: | ||
| contents.append({ | ||
| "path": item_path, | ||
| "name": item, | ||
| "type": "other", | ||
| "content": None, | ||
| "completed": True | ||
| }) | ||
| return contents | ||
|
|
||
| elif os.path.isfile(abs_path): | ||
| with open(abs_path, "r", encoding="utf-8") as file: | ||
| file_content = file.read() | ||
| return { | ||
| "path": abs_path, | ||
| "name": os.path.basename(abs_path), | ||
| "type": "file", | ||
| "content": file_content, #path is a file, content is loaded | ||
| "completed": True | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and large file protection.
The implementation needs additional error handling for robustness:
- File encoding issues
- Permission errors
- Memory constraints with large files
Apply this diff to improve error handling:
def _get_contents(self, path: str) -> Union[List[dict], dict]:
if not isinstance(path, str):
raise TypeError(f"Expected path to be a string, got {type(path).__name__}")
if path == "/":
path = ""
abs_path = os.path.abspath(path)
if not os.path.exists(abs_path):
raise FileNotFoundError(f"Path '{abs_path}' does not exist.")
+
+ if not os.access(abs_path, os.R_OK):
+ raise PermissionError(f"No read permission for '{abs_path}'")
if os.path.isdir(abs_path):
contents = []
for item in os.listdir(abs_path):
item_path = os.path.join(abs_path, item)
+ try:
if os.path.isdir(item_path):
contents.append({
"path": item_path,
"name": item,
"type": "dir",
"content": None,
"completed": True
})
elif os.path.isfile(item_path):
+ # Skip files larger than 10MB
+ if os.path.getsize(item_path) > 10 * 1024 * 1024:
+ logger.warning(f"Skipping large file: {item_path}")
+ continue
contents.append({
"path": item_path,
"name": item,
"type": "file",
"content": None,
"completed": False
})
else:
contents.append({
"path": item_path,
"name": item,
"type": "other",
"content": None,
"completed": True
})
+ except OSError as e:
+ logger.error(f"Error accessing {item_path}: {e}")
+ continue
return contents
elif os.path.isfile(abs_path):
+ # Skip files larger than 10MB
+ if os.path.getsize(abs_path) > 10 * 1024 * 1024:
+ raise ValueError(f"File too large: {abs_path}")
+
+ try:
with open(abs_path, "r", encoding="utf-8") as file:
file_content = file.read()
+ except UnicodeDecodeError:
+ logger.warning(f"Non-UTF-8 file detected: {abs_path}")
+ return {
+ "path": abs_path,
+ "name": os.path.basename(abs_path),
+ "type": "file",
+ "content": None,
+ "completed": False,
+ "error": "Non-UTF-8 encoding"
+ }
return {
"path": abs_path,
"name": os.path.basename(abs_path),
"type": "file",
"content": file_content,
"completed": True
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _get_contents(self, path: str) -> Union[List[dict], dict]: | |
| """ | |
| If the path is a directory, it returns a list of dictionaries, | |
| each representing a file or subdirectory. If the path is a file, its content is read and returned. | |
| :param path: Relative or absolute path within the local repository. | |
| :return: A dict if the path is a file (with file content loaded), or a list of dicts if the path is a directory. | |
| """ | |
| if not isinstance(path, str): | |
| raise TypeError(f"Expected path to be a string, got {type(path).__name__}") | |
| if path == "/": | |
| path = "" | |
| abs_path = os.path.abspath(path) | |
| if not os.path.exists(abs_path): | |
| raise FileNotFoundError(f"Path '{abs_path}' does not exist.") | |
| if os.path.isdir(abs_path): | |
| contents = [] | |
| for item in os.listdir(abs_path): | |
| item_path = os.path.join(abs_path, item) | |
| if os.path.isdir(item_path): | |
| contents.append({ | |
| "path": item_path, | |
| "name": item, | |
| "type": "dir", | |
| "content": None, #path is a dir, content is not loaded | |
| "completed": True | |
| }) | |
| elif os.path.isfile(item_path): | |
| contents.append({ | |
| "path": item_path, | |
| "name": item, | |
| "type": "file", | |
| "content": None, | |
| "completed": False | |
| }) | |
| else: | |
| contents.append({ | |
| "path": item_path, | |
| "name": item, | |
| "type": "other", | |
| "content": None, | |
| "completed": True | |
| }) | |
| return contents | |
| elif os.path.isfile(abs_path): | |
| with open(abs_path, "r", encoding="utf-8") as file: | |
| file_content = file.read() | |
| return { | |
| "path": abs_path, | |
| "name": os.path.basename(abs_path), | |
| "type": "file", | |
| "content": file_content, #path is a file, content is loaded | |
| "completed": True | |
| } | |
| def _get_contents(self, path: str) -> Union[List[dict], dict]: | |
| """ | |
| If the path is a directory, it returns a list of dictionaries, | |
| each representing a file or subdirectory. If the path is a file, its content is read and returned. | |
| :param path: Relative or absolute path within the local repository. | |
| :return: A dict if the path is a file (with file content loaded), or a list of dicts if the path is a directory. | |
| """ | |
| if not isinstance(path, str): | |
| raise TypeError(f"Expected path to be a string, got {type(path).__name__}") | |
| if path == "/": | |
| path = "" | |
| abs_path = os.path.abspath(path) | |
| if not os.path.exists(abs_path): | |
| raise FileNotFoundError(f"Path '{abs_path}' does not exist.") | |
| if not os.access(abs_path, os.R_OK): | |
| raise PermissionError(f"No read permission for '{abs_path}'") | |
| if os.path.isdir(abs_path): | |
| contents = [] | |
| for item in os.listdir(abs_path): | |
| item_path = os.path.join(abs_path, item) | |
| try: | |
| if os.path.isdir(item_path): | |
| contents.append({ | |
| "path": item_path, | |
| "name": item, | |
| "type": "dir", | |
| "content": None, # path is a dir, content is not loaded | |
| "completed": True | |
| }) | |
| elif os.path.isfile(item_path): | |
| # Skip files larger than 10MB | |
| if os.path.getsize(item_path) > 10 * 1024 * 1024: | |
| logger.warning(f"Skipping large file: {item_path}") | |
| continue | |
| contents.append({ | |
| "path": item_path, | |
| "name": item, | |
| "type": "file", | |
| "content": None, | |
| "completed": False | |
| }) | |
| else: | |
| contents.append({ | |
| "path": item_path, | |
| "name": item, | |
| "type": "other", | |
| "content": None, | |
| "completed": True | |
| }) | |
| except OSError as e: | |
| logger.error(f"Error accessing {item_path}: {e}") | |
| continue | |
| return contents | |
| elif os.path.isfile(abs_path): | |
| # Skip files larger than 10MB | |
| if os.path.getsize(abs_path) > 10 * 1024 * 1024: | |
| raise ValueError(f"File too large: {abs_path}") | |
| try: | |
| with open(abs_path, "r", encoding="utf-8") as file: | |
| file_content = file.read() | |
| except UnicodeDecodeError: | |
| logger.warning(f"Non-UTF-8 file detected: {abs_path}") | |
| return { | |
| "path": abs_path, | |
| "name": os.path.basename(abs_path), | |
| "type": "file", | |
| "content": None, | |
| "completed": False, | |
| "error": "Non-UTF-8 encoding" | |
| } | |
| return { | |
| "path": abs_path, | |
| "name": os.path.basename(abs_path), | |
| "type": "file", | |
| "content": file_content, | |
| "completed": True | |
| } |
|
@kinshuksinghbist did you verify that the output of this is the same format as the remote server output? |
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/modules/code_provider/local_repo/local_repo_service.py (1)
274-334:⚠️ Potential issueAdd error handling and large file protection.
While the base implementation is solid, it needs additional error handling for robustness as previously suggested.
The implementation should include:
- File encoding issues
- Permission errors
- Memory constraints with large files
Please refer to the previous review comment for the detailed implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/modules/code_provider/local_repo/local_repo_service.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pre-commit
app/modules/code_provider/local_repo/local_repo_service.py
[warning] 160-184: Code does not adhere to PEP 8 standards. Please format the code using Black.
🔇 Additional comments (2)
app/modules/code_provider/local_repo/local_repo_service.py (2)
6-6: LGTM!The addition of
Unionto the typing imports is appropriate for the new method's return type annotation.
155-155: LGTM!The changes to use dictionary access instead of attribute access are consistent with the new
_get_contentsmethod's return type.Also applies to: 172-172, 175-175, 178-178, 187-188
| contents = [ | ||
| item | ||
| for item in contents | ||
| if item.type == "dir" | ||
| or not any(item.name.endswith(ext) for ext in exclude_extensions) | ||
| ] | ||
| item | ||
| for item in contents | ||
| if item['type'] == "dir" | ||
| or not any(item['name'].endswith(ext) for ext in exclude_extensions) | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix indentation to comply with PEP 8.
The list comprehension's indentation doesn't follow PEP 8 standards. Please adjust the indentation.
Apply this diff to fix the indentation:
- contents = [
- item
- for item in contents
- if item['type'] == "dir"
- or not any(item['name'].endswith(ext) for ext in exclude_extensions)
- ]
+ contents = [
+ item
+ for item in contents
+ if item['type'] == "dir"
+ or not any(item['name'].endswith(ext) for ext in exclude_extensions)
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contents = [ | |
| item | |
| for item in contents | |
| if item.type == "dir" | |
| or not any(item.name.endswith(ext) for ext in exclude_extensions) | |
| ] | |
| item | |
| for item in contents | |
| if item['type'] == "dir" | |
| or not any(item['name'].endswith(ext) for ext in exclude_extensions) | |
| ] | |
| contents = [ | |
| item | |
| for item in contents | |
| if item['type'] == "dir" | |
| or not any(item['name'].endswith(ext) for ext in exclude_extensions) | |
| ] |
Fixes #261
added method _get_content(), to fix the local repo parsing issue.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor