-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: combine custom command implementations from #47 and #53 #59
base: main
Are you sure you want to change the base?
Conversation
- Add 'custom' as valid runtime type - Add required command and args fields for custom runtime - Update validation in pr-check.js - Add custom runtime support in ConfigManager - Update type definitions and interfaces Co-Authored-By: Michael Latman <[email protected]>
Co-Authored-By: Michael Latman <[email protected]>
Co-Authored-By: Michael Latman <[email protected]>
Co-Authored-By: Michael Latman <[email protected]>
Co-Authored-By: Michael Latman <[email protected]>
- Add proper fs module mocking for ESM environment - Fix test state management between runs - Add comprehensive error handling tests - Update ConfigManager to use dynamic fs imports Co-Authored-By: Michael Latman <[email protected]>
# Conflicts: # src/utils/config-manager.ts Co-Authored-By: Michael Latman <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
Add "(aside)" to your comment to have me ignore it. |
❌ No new packages found or validation failed. |
✅ PR validation passed successfully! |
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.
Where are the tests?
src/types/package.ts
Outdated
@@ -23,6 +23,10 @@ export interface PackageHelper { | |||
}; | |||
configureEnv?: (config: any) => Promise<void>; | |||
runtime?: 'node' | 'python'; |
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.
Runtime should be custom right?
src/utils/config-manager.ts
Outdated
serverConfig.command = 'npx'; | ||
serverConfig.args = ['-y', pkg.name]; | ||
if (customCommand) { | ||
serverConfig.command = customCommand.command; |
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.
Custom command should be handled in its own rumtime
} else if (pkg.runtime === 'python') { | ||
serverConfig.command = 'uvx'; |
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.
Looks like we can basically just resolve the command and args based on runtime and custom command and then just set serverCOnfig.command and args based on that versus these if branches
Co-Authored-By: Michael Latman <[email protected]>
❌ No new packages found or validation failed. |
✅ PR validation passed successfully! |
Co-Authored-By: Michael Latman <[email protected]>
❌ No new packages found or validation failed. |
✅ PR validation passed successfully! |
This PR incorporates changes from #47 and #53, combining the custom command functionality from both PRs.
Original implementation in #47 by @matildepark
Additional changes from #53
Changes:
Link to Devin run: https://app.devin.ai/sessions/88d54527369a462c8710f0fb5732ae71