refactor(autostart): remove legacy guards, require lich 5.17.2+#2342
Open
MahtraDR wants to merge 2 commits into
Open
refactor(autostart): remove legacy guards, require lich 5.17.2+#2342MahtraDR wants to merge 2 commits into
MahtraDR wants to merge 2 commits into
Conversation
Core lich 5.17.2+ provides get_settings, start_scripts_if_available, and Map.apply_wayto_overrides natively. Remove the respond_to? guards and the entire legacy DR dependency install path that predated these. Changes: - Remove legacy DR dependency path (first-run install, old lich fallback) - Remove respond_to?(:get_settings) guard (always true on 5.17.2+) - Remove Map.respond_to?(:apply_wayto_overrides) guard (always true) - Remove Gem::Version lich5-update check (5.17.2 > 5.6.2 always) - Consolidate lich5-update into the skip list - Update comment: dependency loads for runtime helpers, not parse_args - Bump version to 0.72, required to Lich >= 5.17.2 - Update spec helpers to match simplified code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
autostart.lic is game-agnostic (game: any). GS players on older lich may not have get_settings, Map.apply_wayto_overrides, or Lich::Util::Update. Restore respond_to? guards for these while keeping the DR legacy path removal (dependency.lic enforces 5.17.2). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAutostart script refactored to simplify DragonRealms (DR) dependency bootstrap from multi-path legacy flow to direct ChangesAutostart Flow Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/autostart.lic (1)
195-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDR dependency-removal branch is unreachable; cleanup/notification never runs.
'dependency'is already in the['infomon', 'repository', 'dependency']set at Line 195, so it always hitsnextat Line 196 before reaching theelsif ... == 'dependency' && XMLData.game =~ /^DR/branch at Line 199. The DR auto-removal and its notification are dead code.Note the spec helper (
spec/autostart/autostart_spec.rbLines 182-213) nests this removal inside the firstifblock beforenext, so the "DR dependency removal" examples (Lines 343-373) pass against the helper but do not reflect production behavior. Aligning production with the helper restores the intended cleanup and removes the false test confidence.🐛 Proposed fix: nest DR removal inside the first conditional
if ['infomon', 'repository', 'dependency'].include?(script_info[:name]) + if script_info[:name] == 'dependency' && XMLData.game =~ /^DR/ + respond "\n--- dependency found in autostart list. Attempting to remove it now. If unsuccessful, it can be safely removed with '#{$clean_lich_char}autostart remove --global dependency'" + temp_script_list = Settings['scripts'] + if (temp_script_list.is_a?(Array)) and (temp_script_info = temp_script_list.find { |s| s[:name] == script_info[:name] }) + temp_script_list.delete(temp_script_info) + Settings['scripts'] = temp_script_list + respond "\n--- #{script_info[:name]} was removed from the global autostart list\n\n" + end + temp_script_list = CharSettings['scripts'] + if (temp_script_list.is_a?(Array)) and (temp_script_info = temp_script_list.find { |s| s[:name] == script_info[:name] }) + temp_script_list.delete(temp_script_info) + CharSettings['scripts'] = temp_script_list + respond "\n--- #{script_info[:name]} was removed from #{XMLData.name}'s autostart list\n\n" + end + end next elsif script_info[:name] == 'lich5-update' && defined?(Lich::Util::Update) next - elsif script_info[:name] == 'dependency' && XMLData.game =~ /^DR/ - respond "\n--- dependency found in autostart list. Attempting to remove it now. If unsuccessful, it can be safely removed with '#{$clean_lich_char}autostart remove --global dependency'" - temp_script_list = Settings['scripts'] - if (temp_script_list.is_a?(Array)) and (temp_script_info = temp_script_list.find { |s| s[:name] == script_info[:name] }) - temp_script_list.delete(temp_script_info) - Settings['scripts'] = temp_script_list - respond "\n--- #{script_info[:name]} was removed from the global autostart list\n\n" - end - temp_script_list = CharSettings['scripts'] - if (temp_script_list.is_a?(Array)) and (temp_script_info = temp_script_list.find { |s| s[:name] == script_info[:name] }) - temp_script_list.delete(temp_script_info) - CharSettings['scripts'] = temp_script_list - respond "\n--- #{script_info[:name]} was removed from #{XMLData.name}'s autostart list\n\n" - end - next else🤖 Prompt for 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. In `@scripts/autostart.lic` around lines 195 - 213, The DR-specific dependency-removal branch is never reached because 'dependency' is already handled by the initial if that immediately calls next; modify the control flow so the DR removal runs before skipping: inside the existing if that checks ['infomon','repository','dependency'] (which references script_info[:name]), add a nested check for script_info[:name] == 'dependency' && XMLData.game =~ /^DR/ and move the cleanup/notification code (the blocks that touch Settings['scripts'], CharSettings['scripts'], and call respond) into that nested branch, then still call next after that nested handling so normal exclusions still skip.spec/autostart/autostart_spec.rb (1)
168-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc comment after the parameter rename.
The signature now takes
has_lich_update:(Line 180) but the doc still documents the removedlich_versionparam, and the "Lines 199-231" reference no longer matches the production loop range.📝 Proposed doc update
-# Replicates lines 199-231 of autostart.lic. +# Replicates the generic Settings/CharSettings autostart loop of autostart.lic. # # `@param` settings_mod [Module] mock for Settings # `@param` char_settings_mod [Module] mock for CharSettings # `@param` script_mod [Module] mock for Script # `@param` xml_data_mod [Module] mock for XMLData -# `@param` lich_version [String] simulated LICH_VERSION +# `@param` has_lich_update [Boolean] whether Lich::Util::Update is available (skip lich5-update when true) # `@param` respond_output [Array<String>] collects warning messages🤖 Prompt for 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. In `@spec/autostart/autostart_spec.rb` around lines 168 - 175, Update the stale doc comment: remove the obsolete `lich_version` parameter entry, add documentation for the new `has_lich_update:` boolean parameter (describe purpose and type), and correct the referenced production line range ("Lines 199-231") to match the actual loop range in autostart.lic; locate the comment block that precedes the spec helper for the autostart replication (the comment that lists params like `settings_mod`, `char_settings_mod`, `script_mod`, `xml_data_mod`, and `respond_output`) and apply these edits so the doc matches the current method signature and source reference.
🤖 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.
Outside diff comments:
In `@scripts/autostart.lic`:
- Around line 195-213: The DR-specific dependency-removal branch is never
reached because 'dependency' is already handled by the initial if that
immediately calls next; modify the control flow so the DR removal runs before
skipping: inside the existing if that checks
['infomon','repository','dependency'] (which references script_info[:name]), add
a nested check for script_info[:name] == 'dependency' && XMLData.game =~ /^DR/
and move the cleanup/notification code (the blocks that touch
Settings['scripts'], CharSettings['scripts'], and call respond) into that nested
branch, then still call next after that nested handling so normal exclusions
still skip.
In `@spec/autostart/autostart_spec.rb`:
- Around line 168-175: Update the stale doc comment: remove the obsolete
`lich_version` parameter entry, add documentation for the new `has_lich_update:`
boolean parameter (describe purpose and type), and correct the referenced
production line range ("Lines 199-231") to match the actual loop range in
autostart.lic; locate the comment block that precedes the spec helper for the
autostart replication (the comment that lists params like `settings_mod`,
`char_settings_mod`, `script_mod`, `xml_data_mod`, and `respond_output`) and
apply these edits so the doc matches the current method signature and source
reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7bae751-0188-4914-a211-d669fb3e4632
📒 Files selected for processing (2)
scripts/autostart.licspec/autostart/autostart_spec.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
respond_to?(:get_settings)andMap.respond_to?(:apply_wayto_overrides)guards -- always true on 5.17.2+Gem::Versionlich5-update version check -- always true on 5.17.2+lich5-updateinto the static skip listNet: -79 lines (+30 / -109)
Companion PR
dependency.lic strip (dr-scripts): elanthia-online/dr-scripts#7407 -- removes all gated dead code, bumps to v4.0.0.
Test plan
bundle exec rspec spec/autostart/autostart_spec.rb-- 49 examples, 0 failures🤖 Generated with Claude Code
Summary by CodeRabbit