-
Notifications
You must be signed in to change notification settings - Fork 64
feat: extension support for recipe command #159
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
base: main
Are you sure you want to change the base?
Conversation
|
Will these extensions be used in rendering somehow? Or it will be a separate PR? |
|
Currently, they are coming through in the JSON output. I'm not sure how best to handle them in other contexts. |
|
Maybe I'm missing something but I don't understand the value of enabling extensions without using them in rendering. Could you please explain me how it works for you? Also for other users it might be confusing why only recipe command have extensions and the rest don't. |
Code Review for PR #159: Extension Support for Recipe CommandOverviewThis PR adds support for specifying desired extensions when using the ✅ Strengths
🔍 Areas for Improvement1. Default Behavior Inconsistency// In PARSER (util/mod.rs)
Extensions::empty() // Default parser uses no extensions
// In ReadArgs
default_values_t = vec![CLIExtensions::None] // CLI defaults to NoneThe comment says "Default: all extensions enabled for legacy use" but the code actually defaults to no extensions. This might break existing workflows that rely on certain extensions. Recommendation: Consider defaulting to 2. Parser Creation EfficiencyCreating a new parser instance for each recipe read operation could be inefficient for batch operations. Suggestion: Consider caching parsers by extension configuration or making the parser creation lazy. 3. Error Handling EnhancementThe Suggestion: Consider warning users when both 4. Documentation GapThe PR doesn't update the main documentation to explain available extensions and their effects. Recommendation: Update
🐛 Potential Issues
🔒 Security ConsiderationsNo security concerns identified. The changes are limited to parser configuration and don't introduce new attack vectors. 🚀 Performance Considerations
✨ Suggestions for Enhancement
enum ExtensionPreset {
Minimal, // No extensions
Standard, // Common extensions
Full, // All extensions
Custom(Vec<CLIExtensions>)
}
📋 Testing Recommendations
Verdict: Approved with Suggestions ✅This is a valuable feature that addresses user needs. The implementation is solid but would benefit from the improvements mentioned above, particularly around documentation and default behavior clarity. Great work on implementing this feature! The code quality is good and follows the project's conventions well. |
|
Agreed that this would be useful to have in the rendering as well, but even the generated JSON would be useful for me as I use the CLI to parse recipes to JSON then use that as input to my own renderer. |
Adds support for specifying desired extensions when using the
recipecommand. At least partially addresses #69 by making use of the alias extension possible.