fix(mister): add RA set names to launchers#852
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)
📝 WalkthroughWalkthroughThis PR extends MiSTer launch behavior with SetName and SetNameSameDir options to control MGL profile naming. Changes flow from LaunchOptions contract through zapscript input, validation utilities, launcher implementations, RetroAchievements wiring, MGL output escaping, and a go-zapscript dependency bump. ChangesMiSTer SetName Launch Options Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/platforms/mister/launchers_test.go`:
- Around line 548-596: Add a new subtest in TestSetNameOptions that exercises
applySetNameOptions when SetName is omitted but SetNameSameDir is provided:
initialize core := cores.Core{SetName: "ExistingName", SetNameSameDir: false}
(or true) and call applySetNameOptions(&core,
&platforms.LaunchOptions{SetNameSameDir: "yes"/"no"}); assert no error and that
core.SetName remains "ExistingName" while core.SetNameSameDir is updated to the
expected boolean; include a negative variant for invalid SetNameSameDir to
assert an error. Ensure the new subtest uses t.Parallel() and references
applySetNameOptions, cores.Core and platforms.LaunchOptions.
In `@pkg/platforms/mister/launchers.go`:
- Around line 127-133: The branch currently only calls applySetNameOptions when
opts.SetName is non-empty, which drops the set_name_same_dir option; change the
condition to check for either opts.SetName != "" or the boolean/flag field (e.g.
opts.SetNameSameDir) so applySetNameOptions(&sn, opts) runs whenever either
option is provided, keeping the existing sn copy/assignment logic (variables:
opts, SetName, SetNameSameDir, s, sn, applySetNameOptions).
- Around line 285-300: The helper applySetNameOptions early-returns when
opts.SetName is empty which prevents processing opts.SetNameSameDir-only
overrides; change the logic so applySetNameOptions first returns only if opts ==
nil, then if opts.SetNameSameDir != "" call parseSetNameSameDir and apply the
result to core.SetNameSameDir, and only afterwards handle opts.SetName (validate
via validSetName and assign core.SetName and default core.SetNameSameDir=false
before applying any SetNameSameDir override); keep using parseSetNameSameDir,
validSetName, and core.SetName/core.SetNameSameDir to locate the needed changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 724f3cd1-cd5d-4fd1-99e1-aa8c68d180d0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modpkg/platforms/mister/launchers.gopkg/platforms/mister/launchers_test.gopkg/platforms/mister/mgls/mgls.gopkg/platforms/mister/mgls/mgls_test.gopkg/platforms/platforms.gopkg/zapscript/launch.gopkg/zapscript/launch_test.go
Summary
Test
Summary by CodeRabbit
New Features
Bug Fixes
Tests