Skip to content

Conversation

@rmecham
Copy link

@rmecham rmecham commented Sep 7, 2025

Adds support for specifying desired extensions when using the recipe command. At least partially addresses #69 by making use of the alias extension possible.

@dubadub
Copy link
Member

dubadub commented Sep 7, 2025

Will these extensions be used in rendering somehow? Or it will be a separate PR?

@rmecham
Copy link
Author

rmecham commented Sep 8, 2025

Currently, they are coming through in the JSON output. I'm not sure how best to handle them in other contexts.

@dubadub
Copy link
Member

dubadub commented Sep 10, 2025

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.

@claude
Copy link

claude bot commented Sep 19, 2025

Code Review for PR #159: Extension Support for Recipe Command

Overview

This PR adds support for specifying desired extensions when using the recipe command, enabling features like component aliases. The implementation is clean and follows the existing codebase patterns well.

✅ Strengths

  1. Good Architecture: The extension handling is well-structured with a clear separation of concerns
  2. Backward Compatibility: Default behavior remains unchanged with Extensions::empty() as the default
  3. Test Coverage: Includes integration test for component alias functionality
  4. Documentation: Clear help text for the new CLI option

🔍 Areas for Improvement

1. 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 None

The 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 CLIExtensions::Compat or document this breaking change clearly.

2. Parser Creation Efficiency

Creating 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 Enhancement

The build_extensions_from_cli function silently handles conflicting options (All vs None).

Suggestion: Consider warning users when both --extension all and --extension none are specified.

4. Documentation Gap

The PR doesn't update the main documentation to explain available extensions and their effects.

Recommendation: Update docs/recipe.md with:

  • List of available extensions
  • Examples of each extension's behavior
  • Migration guide for users relying on previous default behavior

🐛 Potential Issues

  1. Visibility Change: Making read module public (pub mod read;) exposes internal implementation details. Consider keeping it private and only exporting CLIExtensions.

  2. Test Snapshot: The snapshot test update shows the test now processes 6 recipes instead of 5, but only one new test recipe with aliases was added. Verify this is intentional.

🔒 Security Considerations

No security concerns identified. The changes are limited to parser configuration and don't introduce new attack vectors.

🚀 Performance Considerations

  • Parser creation per recipe read could impact performance for batch operations
  • Consider benchmarking the impact on large recipe collections

✨ Suggestions for Enhancement

  1. Add extension presets:
enum ExtensionPreset {
    Minimal,    // No extensions
    Standard,   // Common extensions
    Full,       // All extensions
    Custom(Vec<CLIExtensions>)
}
  1. Add validation for incompatible extensions (if any exist in the cooklang parser)

  2. Consider adding a config file option for default extensions to avoid repetitive CLI arguments

📋 Testing Recommendations

  1. Add tests for:

    • Multiple extensions enabled simultaneously
    • Conflicting extension specifications
    • Stdin input with extensions
    • Error cases with invalid extension combinations
  2. Add performance benchmarks comparing parser creation overhead

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.

@calebhearth
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants