- Notifications
You must be signed in to change notification settings - Fork3.2k
Improvements & refactoring of get_file_contents#1582
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR refactors theget_file_contents tool to eliminate the requirement for directory paths to end with a trailing slash. The refactoring unifies the code path for both files and directories by first calling the GitHub Contents API to determine the type, then proceeding accordingly.
Key Changes:
- Removed the trailing slash requirement from the path parameter description
- Unified the GetContents API call for both files and directories instead of having separate conditional logic
- Simplified control flow by checking the response type (file vs directory) after the API call rather than before
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/github/repositories.go | Refactored GetFileContents function to remove trailing slash dependency and unify the API call path for files and directories; updated parameter description |
| pkg/github/toolsnaps/get_file_contents.snap | Updated tool schema snapshot to reflect the changed path parameter description |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tommaso-moro left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
left a comment but otherwise lgtm!
Uh oh!
There was an error while loading.Please reload this page.
f4a7585f4a7585 toa3460a9Compare
This PR contains refactoring of get_file_contents tool that eliminates error
file content SHA is nil, if a directory was requested, path parameters should end with a trailing slash '/'We shouldn't fail when trailing slash is not supplied. Also, for both directories and files the same API call is being used, therefore there's no need for two separate logical paths.
I removed the reliance on trailing slash and branching related to it. Instead, if GetContents returns directory content - we return it, if it is a file - we proceed with downloading etc.