feat(generate): add language and instructions to mind maps#264
feat(generate): add language and instructions to mind maps#264voidborne-d wants to merge 2 commits intoteng-lin:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to specify an output language and optional generation instructions when creating mind maps. These changes affect the generate_mind_map API method and the CLI, which now includes a new positional argument for descriptions and a --language option. Unit tests have been updated to cover these new features. The review feedback suggests refactoring the CLI logic to resolve the language and instructions once before the conditional output block to avoid redundant processing and improve maintainability.
src/notebooklm/cli/generate.py
Outdated
| language=resolve_language(language), | ||
| instructions=description or None, | ||
| ) | ||
| else: | ||
| with console.status("Generating mind map..."): | ||
| result = await client.artifacts.generate_mind_map( | ||
| nb_id_resolved, source_ids=sources | ||
| nb_id_resolved, | ||
| source_ids=sources, | ||
| language=resolve_language(language), |
There was a problem hiding this comment.
The logic for calling generate_mind_map is duplicated across the if json_output branches, including redundant calls to resolve_language(language) and description or None. Since resolve_language may involve reading the configuration file, it's more efficient to resolve these values once before the conditional block. This would also simplify the code and improve maintainability.
References
- To improve efficiency and clarity, avoid reading the same file multiple times within a single function. Instead, read the file once, store its contents in a variable, and reuse the variable.
Summary
languageandinstructionssupport toArtifactsAPI.generate_mind_map()generate mind-map [DESCRIPTION] --language ...in the CLICloses #250
Testing
Summary by CodeRabbit
New Features
generate mind-mapadds an optionaldescriptionargument and a--languageoption.Tests