Fix(eforgery.lic) - Major overhaul with updates

[AndreasWelch]

v1.3
Removed deposit all from withdrawal routine, since it deposits the note we have and resets that
Further improved silver handling to avoid unnecessary bank trips
Fixed processing of slabs/blocks where they are already the correct size
Reworked messaging to use Lich::Messaging instead of puts/respond/echo
Significantly expanded in-line documentation

Note: Unfortunately I apparently forgot I had this version no committed, so it's out of sync with an update that happened after (which is going to force a manual merge). I think I've effectively sync'd it, though we had a different solution to cleaning up old notes.

---

Important

Major overhaul of eforgery.lic script with improved silver handling, messaging updates, and expanded documentation.

• Behavior:
  • Removed "deposit all" from withdrawal routine to prevent note reset.
  • Improved silver handling to minimize unnecessary bank trips.
  • Fixed slab/block processing for correct size detection.
  • Updated AFK script guard to pause instead of waiting for level prompt.
• Messaging:
  • Replaced puts/respond/echo with Lich::Messaging for consistent messaging.
  • Added detailed in-line documentation for better code understanding.
• Misc:
  • Added debug mode to toggle detailed logging.
  • Updated GUI setup to reflect new settings and tooltips.
  • Enhanced error handling and user prompts throughout the script.

^{This description was created by for 2aa9553. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 57040b4 in 2 minutes and 15 seconds. Click for details.

• Reviewed 1483 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 12 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. scripts/eforgery.lic:666

• Draft comment:
  In the read_note method the variable ‘result’ is only assigned if a match is found. If no line matches the regex, ‘result’ remains undefined. Consider initializing it (e.g. to nil) for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. scripts/eforgery.lic:613

• Draft comment:
  Multiple methods use 'exit' to abort execution (e.g. in withdraw and get_note). Abrupt termination may bypass cleanup hooks. Consider using exception handling or controlled shutdown to allow proper cleanup.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is about the use of exit potentially bypassing cleanup hooks. However, in Lich scripting (which this appears to be), before_dying hooks are specifically designed to run when a script exits, including when exit is called. The comment doesn't provide evidence that cleanup hooks are actually being bypassed. It's making a speculative suggestion about "consider using exception handling" without demonstrating that there's an actual problem. The diff shows this is existing behavior that wasn't changed - the exit calls were already there. This appears to be speculative advice rather than identifying a concrete bug introduced by the changes. The comment might be correct if before_dying hooks don't actually run on exit in this framework, but I don't have definitive proof either way. The script does register cleanup hooks, so the author was clearly thinking about cleanup. Without evidence that cleanup is actually failing, this might just be theoretical concern. While I can't be 100% certain about the Lich framework's behavior, the comment is speculative ("may bypass") and suggests a refactor without demonstrating an actual problem. The rules state not to make speculative comments. Additionally, this code pattern with exit appears to be pre-existing (not introduced in this diff), so commenting on it violates the rule about only commenting on changes. This comment should be deleted. It's speculative advice about potential issues with exit bypassing cleanup hooks, but doesn't demonstrate an actual problem. The code uses before_dying hooks which are designed to handle cleanup on exit. More importantly, the exit calls appear to be pre-existing code, not changes introduced in this diff.

3. scripts/eforgery.lic:1363

• Draft comment:
  The glyph method uses 'line = get' to capture command output but it’s unclear if 'get' is defined in this scope. Verify that 'get' is appropriate or consider using waitfor consistently.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. scripts/eforgery.lic:1271

• Draft comment:
  The cut_once method’s logic is complex and in some branches it sets left/right to nil. Ensure that callers properly handle these nil values and that all edge cases are tested.
• Reason this comment was not posted:
  Comment was on unchanged code.

5. scripts/eforgery.lic:580

• Draft comment:
  There is extensive repeated use of Lich::Messaging.msg with similar patterns. Consider refactoring these calls into a helper method to reduce duplication and improve maintainability.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a code quality refactoring suggestion. According to the rules, "Comments that suggest code quality refactors are good! But only if they are actionable and clear." The suggestion is somewhat actionable (create a helper method) but not very specific about what the helper should look like or where it should be placed. The comment is about changes that were made (the diff shows extensive conversion from echo/respond to Lich::Messaging.msg), so it is about the changes. However, the comment is quite vague - it doesn't specify what the helper method signature should be, where to put it, or provide an example. It's more of a general observation than a concrete, actionable suggestion. The comment is somewhat vague and doesn't provide concrete guidance on implementation. It's also debatable whether this refactoring would actually improve the code - the current pattern is clear and explicit, and a helper method might obscure what's happening. The repeated pattern is intentional and follows Ruby conventions for conditional logging. While the comment lacks specificity, it does point to a legitimate pattern that could be refactored. However, given the rules state comments should be "actionable and clear," and this comment doesn't provide enough detail about how to implement the refactoring, it falls short of that standard. The current code is readable and the pattern is consistent throughout. This comment should be deleted. While it identifies a pattern, it's not sufficiently actionable or clear about what the refactoring should look like. The current code is readable and consistent, and the comment doesn't provide enough concrete guidance to be useful.

6. scripts/eforgery.lic:138

• Draft comment:
  Typo: Consider changing "will auto-determined" to "will be auto-determined" or "will be automatically determined."
• Reason this comment was not posted:
  Comment was on unchanged code.

7. scripts/eforgery.lic:143

• Draft comment:
  Typographical error: "thats" should be "that's".
• Reason this comment was not posted:
  Comment was on unchanged code.

8. scripts/eforgery.lic:178

• Draft comment:
  There appear to be a couple of typographical issues in the string on this line: 1. "Unchek" looks like a typo – it should likely be "Uncheck". 2. "MARKSs" may also be a typo; consider verifying if it should be "marks" (or formatted differently). Please review these for correctness.
• Reason this comment was not posted:
  Comment was on unchanged code.

9. scripts/eforgery.lic:854

• Draft comment:
  Typo detected: In the inline comment, "rent requires silvers specifically" might be intended to read "rent requires silver specifically."
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. scripts/eforgery.lic:1150

• Draft comment:
  Typographical note: The message string "Your glyph requires #{@SiZe} pound blocks. saving info..." contains two spaces after the period and uses a lowercase 'saving'. Consider revising it to use a single space and proper capitalization (e.g., "Your glyph requires #{@SiZe} pound blocks. Saving info...") for consistency in user-facing messages.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. scripts/eforgery.lic:1722

• Draft comment:
  Typo: In the example for setting material, "[steel slab 4][maoral block 3]" seems to contain a misspelling. Consider correcting "maoral" to the intended word (e.g. "moral").
• Reason this comment was not posted:
  Comment was on unchanged code.

12. scripts/eforgery.lic:1734

• Draft comment:
  Typo: The phrase "Do NOT use commas in when entering Settings for glyph and material" has an extra "in". Consider changing it to "Do NOT use commas when entering Settings for glyph and material".
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a valid typo correction in user-facing documentation/help text. The phrase "Do NOT use commas in when entering Settings" should read "Do NOT use commas when entering Settings". However, I need to check if this is about a change made in the diff. Looking at the diff, line 1734 is part of the usage instructions that were converted from respond to Lich::Messaging.mono. The actual text content with the typo was already present before this PR - it wasn't introduced by this change. The PR only changed how the message is displayed (the messaging method), not the content itself. According to the rules, comments should be about changes made by the diff, not pre-existing issues in unchanged code. While the typo is real and should be fixed, the typo existed before this PR. The diff only changed the messaging method from respond to Lich::Messaging.mono, not the actual text content. The comment is technically about unchanged content, just displayed differently. Actually, looking more carefully at the diff, the line WAS changed - it was converted from one format to another. Even though the text content is the same, the line itself was modified as part of the PR's changes to update messaging. This could be considered "about the changes" since the author touched this line. However, the spirit of the rule is to avoid commenting on pre-existing issues that weren't introduced by the PR. This comment identifies a legitimate typo, but the typo existed before this PR. The PR only changed the messaging method, not the text content. Since the comment is about pre-existing content rather than something introduced or meaningfully changed by this PR, it should be deleted per the rules.

Workflow ID: wflow_VzRTJqYNSK9bL0Lo

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[Copilot]

Pull request overview

This PR represents a major overhaul of the eforgery.lic forging script, introducing significant improvements to code quality, messaging, and silver handling. The update refactors the codebase to use modern Lich scripting patterns while adding comprehensive inline documentation.

Key Changes:

• Migrated all output from puts/respond/echo to Lich::Messaging for consistent, structured logging
• Improved silver handling to minimize unnecessary bank trips by checking current silver before withdrawing
• Replaced deprecated start_script/wait_while patterns with Script.run throughout
• Added extensive method-level documentation with YARD-style docstrings
• Fixed slab/block processing edge cases and added better handling for "already correct size" scenarios

Comments suppressed due to low confidence (1)

scripts/eforgery.lic:906

• The done variable is commented out but not removed. This appears to be dead code that should either be removed entirely or uncommented if still needed for logic flow.

      # done = line =~ /shake your head and /

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[AndreasWelch]

@mrhoribu This is a change from your original which deposted only the known note. I changed it to all so it'll deposit everything (including other unknown notes) and then start fresh so we should only have the known note. However, we can revert this change if I'm missing something in the intent here.

[AndreasWelch]

Addl note, github is just being weird with 623. The line isn't deleted, it's just got another match added to work for mist harbor.

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 2aa9553 in 2 minutes and 30 seconds. Click for details.

• Reviewed 1405 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. scripts/eforgery.lic:715

• Draft comment:
  Review the 'deposit all' command in get_note. Ensure that depositing all silver does not inadvertently deposit the promissory note if it’s already held. Confirm that this behavior is intentional given your recent changes.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that a specific behavior is intentional, which is against the rules. It doesn't provide a specific code suggestion or ask for a test to be written. It also doesn't point out a specific issue or suggest an improvement.

2. scripts/eforgery.lic:1115

• Draft comment:
  Ensure consistency in regex matching for container state in you_get. The code checks for "It's closed" without a period in one condition and with a period in another, which might lead to mismatches.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

3. scripts/eforgery.lic:1715

• Draft comment:
  Usage instructions now employ Lich::Messaging.xml_encode for backwards compatibility. Please verify that these messages display properly across older and newer versions of Lich.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

4. scripts/eforgery.lic:614

• Draft comment:
  There's a possibly unintended escaped double quote at the end of the string ("you don't seem to have that much in the account.""). Please verify if the extra backslash is needed or should be removed.
• Reason this comment was not posted:
  Comment was on unchanged code.

5. scripts/eforgery.lic:1379

• Draft comment:
  Typo: The regex alternative contains a double space after the period ('has already been worked on. It cannot be scribed.') and an inconsistent capitalization ('It cannot' vs 'it cannot') compared to the first alternative. Please consider using consistent spacing and case.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a code quality comment about a regex pattern. The comment is about a change that was made in the diff (line 1379 in the new file). The developer added an alternative pattern with | to match two slightly different messages. The question is: are these two different actual game messages, or is one a typo? Without access to the game's actual output messages, I cannot definitively say whether both patterns are needed. However, the fact that the developer explicitly added both alternatives suggests they encountered both messages in the game. The double space and capitalization difference could be intentional to match actual game output variations. This type of defensive regex matching is common when dealing with inconsistent game output. The comment is speculative ("Please consider") rather than definitively stating there's an error. I don't have access to the actual game messages to verify whether both regex alternatives are legitimate variations that occur in the game. The developer may have intentionally added both patterns because they encountered both messages. The comment is asking them to "consider" consistency but doesn't provide evidence that this is actually wrong. While I can't verify the game's actual output, the comment is making an assumption that this is a typo rather than intentional. Given that the developer explicitly added both alternatives (this wasn't accidental duplication), they likely had a reason. The comment is speculative and asks the developer to verify something they probably already considered when writing the code. This falls under "asking the PR author to confirm their intention" which the rules say to avoid. This comment is speculative and asks the developer to verify/consider something without strong evidence that it's actually wrong. The developer likely intentionally added both regex alternatives to match different game message variations. The comment should be deleted as it's asking the author to double-check their work without clear evidence of an error.

6. scripts/eforgery.lic:1724

• Draft comment:
  Typographical error: 'maoral block 3' seems incorrect. Did you mean 'material block 3' or another intended term?
• Reason this comment was not posted:
  Comment was on unchanged code.

7. scripts/eforgery.lic:1736

• Draft comment:
  Typographical error: The phrase 'Do NOT use commas in when entering Settings for glyph and material' likely should be 'Do NOT use commas when entering Settings for glyph and material'.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_02pieibFPp76r89P

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}