Update language options in nodes_ace.py#12578
Update language options in nodes_ace.py#12578iChristGit wants to merge 4 commits intoComfy-Org:masterfrom
Conversation
Modified it to include all 51 language options ace-step1.5 supports instead of the original 23 comfyui had.
iChristGit
requested review from
Kosinkadink,
comfyanonymous and
guill
as code owners
📝 WalkthroughWalkthroughThe PR updates the 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_extras/nodes_ace.py`:
- Line 45: The language Combo input io.Combo.Input("language", options=[...])
currently has no default so the UI selects the first option ('ar'); add an
explicit default parameter (e.g., default='en' or default='unknown') to the
io.Combo.Input call to set a sensible initial selection for users and avoid 'ar'
being implicitly chosen.
| io.Float.Input("duration", default=120.0, min=0.0, max=2000.0, step=0.1), | ||
| io.Combo.Input("timesignature", options=['2', '3', '4', '6']), | ||
| io.Combo.Input("language", options=["en", "ja", "zh", "es", "de", "fr", "pt", "ru", "it", "nl", "pl", "tr", "vi", "cs", "fa", "id", "ko", "uk", "hu", "ar", "sv", "ro", "el"]), | ||
| io.Combo.Input("language", options=['ar', 'az', 'bg', 'bn', 'ca', 'cs', 'da', 'de', 'el', 'en', 'es', 'fa', 'fi', 'fr', 'he', 'hi', 'hr', 'ht', 'hu', 'id', 'is', 'it', 'ja', 'ko', 'la', 'lt', 'ms', 'ne', 'nl', 'no', 'pa', 'pl', 'pt', 'ro', 'ru', 'sa', 'sk', 'sr', 'sv', 'sw', 'ta', 'te', 'th', 'tl', 'tr', 'uk', 'ur', 'vi', 'yue', 'zh', 'unknown']), |
There was a problem hiding this comment.
Consider adding an explicit default to avoid 'ar' becoming the implicit first-pick.
No default= is specified on the language combo, so the UI will fall back to the first list element — 'ar' (Arabic). For a music-generation tool, 'en' or 'unknown' is likely a more practical starting point for most users.
💡 Suggested fix
- io.Combo.Input("language", options=['ar', 'az', 'bg', ..., 'unknown']),
+ io.Combo.Input("language", options=['ar', 'az', 'bg', ..., 'unknown'], default='unknown'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@comfy_extras/nodes_ace.py` at line 45, The language Combo input
io.Combo.Input("language", options=[...]) currently has no default so the UI
selects the first option ('ar'); add an explicit default parameter (e.g.,
default='en' or default='unknown') to the io.Combo.Input call to set a sensible
initial selection for users and avoid 'ar' being implicitly chosen.
|
The languages were ordered by popularity, I want the default to be 'en' not 'ar' |
changed order of the languages to be ordered by popularity en is default unknown is last
|
List of languages has been re-arranged by popularity |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
comfy_extras/nodes_ace.py (1)
45-45: LGTM — 'en' is now the implicit default, resolving the previous concern.The expanded list (51 entries, popularity-ordered,
'unknown'last) looks correct and consistent with the node's existing combo-input pattern. No breaking changes: every language from the old list is still present, and'en'is now the first element, so it becomes the implicit default as requested.One optional hardening step: add an explicit
default='en'so the intended default is unambiguously declared and immune to future list reorderings.💡 Optional hardening
- io.Combo.Input("language", options=['en', 'zh', 'es', ...]), + io.Combo.Input("language", options=['en', 'zh', 'es', ...], default='en'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_extras/nodes_ace.py` at line 45, The combo input for language currently relies on list order to make 'en' the implicit default; explicitly set the default to avoid breakage if the options list is reordered by adding a default='en' argument to the io.Combo.Input call (the invocation io.Combo.Input("language", options=[...]) should include default='en') so the node's intended default is unambiguous and resilient to future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@comfy_extras/nodes_ace.py`:
- Line 45: The combo input for language currently relies on list order to make
'en' the implicit default; explicitly set the default to avoid breakage if the
options list is reordered by adding a default='en' argument to the
io.Combo.Input call (the invocation io.Combo.Input("language", options=[...])
should include default='en') so the node's intended default is unambiguous and
resilient to future changes.
Test Evidence CheckIf this PR modifies behavior that requires testing, a test explanation is required. PRs lacking applicable test explanations may not be reviewed until added. Please add test explanations to ensure code quality and prevent regressions. If this PR changes user-facing behavior, visual proof (screen recording or screenshot) is required. PRs without applicable visual documentation may not be reviewed until provided. You can add it by:
|
|
@comfyanonymous The only changes were adding all the available language codes from Ace-step1.5, sorted by popularity. 🙏 |
3 participants
Modified it to include all 51 language options ace-step1.5 supports instead of the original 23 comfyui had.