-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix requesty model listing #7378
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
Fix requesty model listing #7378
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.
Thank you for your contribution! The fix correctly addresses the issue described in #7377. I've reviewed the changes and have one suggestion for improvement.
|
|
||
| const resolvedBaseUrl = toRequestyServiceUrl(baseUrl) | ||
| const modelsUrl = new URL("models", resolvedBaseUrl) | ||
| const modelsUrl = new URL("v1/models", resolvedBaseUrl) |
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.
The fix looks good! Changing to correctly ensures the proper URL construction.
However, I noticed there are no dedicated unit tests for the function. Would it be beneficial to add test coverage for this URL construction logic to prevent regression? This could help ensure the fix continues to work correctly with various base URL formats.
|
|
||
| const resolvedBaseUrl = toRequestyServiceUrl(baseUrl) | ||
| const modelsUrl = new URL("models", resolvedBaseUrl) | ||
| const modelsUrl = new URL("v1/models", resolvedBaseUrl) |
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.
The fix looks good! Changing to "v1/models" correctly ensures the proper URL construction.
However, I noticed there are no dedicated unit tests for the getRequestyModels function. Would it be beneficial to add test coverage for this URL construction logic to prevent regression? This could help ensure the fix continues to work correctly with various base URL formats.
mrubens
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.
Thanks for the fix! Maybe we or @roomote-agent can open a new PR with a test as a follow-up.
Related GitHub Issue
Closes: #7377
Roo Code Task Context (Optional)
Description
Fix the model listing URL for Requesty
Test Procedure
Check that the model listing URL ends with /v1/models, not just /models.
Pre-Submission Checklist
Screenshots / Videos
N/A
Documentation Updates
N/A
Additional Notes
Please help us merge this into nightly before the current nightly hits prod.
Important
Fixes the model listing URL in
getRequestyModelsto use/v1/modelsendpoint.getRequestyModelsfunction inrequesty.tsto use/v1/modelsinstead of/models.spotty-queens-crash.mdfor versioning.This description was created by
for aca6592. You can customize this summary. It will automatically update as commits are pushed.