feat(fletchit.lic) - Complete Rewrite to Modernized, Modularize, and Fix Various Bugs

[AndreasWelch]

modularized, modernized, took care of some bugs

---

Important

Complete rewrite of fletchit.lic script with modular architecture, improved error handling, and enhanced fletching features.

• Behavior:
  • Complete rewrite of fletchit.lic with modular architecture.
  • Supports automated fletching of arrows, light bolts, and heavy bolts with painting and cresting.
  • Implements learning mode to pause when mind is above a threshold.
  • Auto-buying of supplies if enabled and mapped room is available.
• Modules:
  • Config: Manages settings, defaults, and versioning.
  • Validator: Validates and normalizes settings.
  • Inventory: Manages container operations and silver checking.
  • GameActions: Provides game command wrappers like casting haste.
  • Shopping: Handles supply checking and automated purchasing.
  • Crafting: Manages the complete arrow/bolt creation process.
  • Bundle: Manages bundling of arrows/bolts.
• Misc:
  • Improved error handling with status symbols.
  • Enhanced messaging consistency and dynamic ammo type messages.
  • Fixed arrow/bolt tracking logic and mind percentage usage.

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

[Copilot]

Copilot wasn't able to review any files in this pull request.

---

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

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 470a68f in 3 minutes and 24 seconds. Click for details.

• Reviewed 2982 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 14 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/fletchit.lic:1525

• Draft comment:
  Typo: 'incorectly' should be 'incorrectly'.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. scripts/fletchit.lic:310

• Draft comment:
  Overuse of 'exit' for error handling; consider raising an exception or returning an error value 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 = 20% vs. threshold = 50% This comment is about code style/architecture rather than a bug. The use of exit is consistent throughout the codebase and appears intentional. The refactor changelog mentions "standardized error handling with status symbols" - which refers to the return symbols like :success, :failed, :no_shafts used in the Crafting module, not the error handling for fatal configuration issues. Using exit for unrecoverable errors (missing container, failed to get knife, etc.) is a reasonable design choice for a script. The comment is speculative ("consider") and suggests a refactor without strong evidence it's wrong. The script already has cleanup mechanisms via before_dying. This is more of a code review opinion than a clear defect. The comment might have merit if there were specific examples of cleanup not happening or resources being leaked. The script does use before_dying hooks which suggests cleanup is considered. Perhaps the author intentionally chose exit for fatal errors vs return values for recoverable states. While the critique about cleanup is valid, the code does show before_dying hooks are used (line 1635, 1483-1486), suggesting cleanup is handled. The comment is a general architectural suggestion without pointing to a specific problem. The refactor already addressed error handling by introducing status symbols for recoverable errors - using exit for unrecoverable configuration errors is a reasonable design choice. This comment should be deleted. It's a general architectural suggestion without evidence of a specific problem. The code already has cleanup mechanisms and uses status symbols for recoverable errors. Using exit for fatal configuration errors is a reasonable design choice for a script, and the comment doesn't identify any actual bug or issue.

3. scripts/fletchit.lic:525

• Draft comment:
  The regex in find_order_number is fragile. It depends on the HTML structure; consider using a more robust parser or additional validations.
• 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 comment is about code quality and suggests the regex parsing is "fragile." However, looking at the rules: 1) This is a refactor/reorganization - the regex parsing logic existed in the old code and was just moved into a module method. 2) The comment doesn't point to a specific bug or failure case - it's speculative ("depends on HTML structure"). 3) The comment suggests "consider using a more robust parser" but doesn't provide concrete evidence that the current approach fails or specific scenarios where it would break. 4) This is a game scripting environment where HTML parsing from game output is common practice. 5) The comment doesn't show that this change introduced a new problem - this parsing approach was already in use. According to the rules, I should only keep comments with STRONG EVIDENCE of correctness, and speculative comments should be removed. The regex might actually be appropriate for this use case - game output tends to be consistent and controlled. The comment could be valid if there's evidence the HTML structure changes frequently or if the regex fails in practice, but I don't see that evidence. However, regex HTML parsing can be legitimately fragile in general. While regex HTML parsing can be fragile in general web contexts, this is parsing controlled game output, not arbitrary HTML. The comment provides no evidence of actual failures or specific scenarios where this breaks. It's a speculative code quality suggestion without demonstrating a real problem. The rules explicitly say "Do NOT make speculative comments" and "you must see STRONG EVIDENCE that the comment is correct." This comment should be deleted. It's a speculative code quality suggestion without evidence of actual problems. The regex parsing was already in the original code and is being used in a controlled game environment where output is consistent. No specific failure cases or bugs are identified.

4. scripts/fletchit.lic:1211

• Draft comment:
  Bundle module: If bundle_ids and bundle_desc lengths do not match, the script exits abruptly. Consider providing a more graceful error handling or recovery strategy.
• 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 suggestion about error handling. The comment says "consider providing a more graceful error handling or recovery strategy" rather than just exiting. However, looking at the context: (1) This is in the bundle_arrows function which is called from the command line with ";FletchIt bundle" - it's a standalone operation, not part of the main fletching loop. (2) If the bundle info can't be retrieved after waiting 6 seconds, there's likely a real problem with the game state or container contents. (3) The error message is clear about what failed. (4) The old code also exited in this scenario. (5) What would a "recovery strategy" even look like here? Retry? Skip bundling? The comment is vague and doesn't provide a specific actionable suggestion. This feels like a speculative "could be better" comment rather than identifying an actual bug or clear improvement. The comment might have merit if there's a common scenario where bundle info retrieval fails temporarily and could succeed on retry. Maybe network lag or game state issues could cause this. However, without knowing the game mechanics, it's hard to say if retry logic would help or just cause more problems. The comment is also quite vague - "more graceful" and "recovery strategy" aren't specific enough to be immediately actionable. While retry logic could theoretically help, the comment doesn't provide evidence that this is a real problem or that the current approach is wrong. The 6-second timeout with wait_until already provides some resilience. For a bundling operation that's run manually by the user, exiting with a clear error message is reasonable. The comment is too vague and speculative to be actionable - it doesn't identify a specific bug or provide a clear improvement path. This comment should be deleted. It's a vague code quality suggestion without specific actionable guidance. The current error handling (clear error message + exit) is reasonable for a manually-invoked bundling operation. The comment doesn't identify a real bug or provide evidence that the current approach causes problems.

5. scripts/fletchit.lic:1492

• Draft comment:
  In the GTK setup, widget types are checked via strict class equality. Consider using duck typing (e.g. checking for required methods) to improve flexibility.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. scripts/fletchit.lic:1672

• Draft comment:
  The check for existing shafts uses an equality on item.noun which may be brittle. Consider using a regex match or standardizing item names to ensure robustness.
• 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% The comment suggests the equality check "may be brittle" but doesn't provide strong evidence that it IS brittle. The code explicitly checks for both "shaft" and "shafts" which covers the expected cases. The reviewer says "consider using a regex match" but elsewhere in the same file (line 485), regex is used for similar checks: item.name =~ /shaft/. So there's inconsistency in the codebase, but the current approach is explicit and clear. The comment is speculative ("may be brittle") rather than identifying a definite bug. Without evidence that item.noun can return unexpected values, this is more of a style suggestion than a bug fix. The rules say not to keep speculative comments. The comment might be valid if there's evidence that item.noun can return values like "shaft-like" or "wooden shaft" that would fail the equality check. However, in Lich/GemStone scripts, item.noun is typically a standardized property that returns just the noun. The code already handles both singular and plural explicitly, which is actually more robust than a loose regex that might match unintended items. While there is some inconsistency in the codebase (regex used elsewhere), the current equality checks are explicit and clear. The comment is speculative ("may be brittle") without providing evidence of actual brittleness. The code already handles the expected variations (singular/plural). This appears to be a style preference rather than a bug that needs fixing. This comment should be deleted. It's speculative ("may be brittle") without strong evidence of an actual problem. The code explicitly handles both singular and plural forms, which is clear and maintainable. The suggestion is more about style/consistency than fixing a bug.

7. scripts/fletchit.lic:1619

• Draft comment:
  Progress reporting hook uses inline time formatting. Consider refactoring this logic into a helper method for improved readability and reuse.
• Reason this comment was not posted:
  Comment was on unchanged code.

8. scripts/fletchit.lic:421

• Draft comment:
  The monitor_interaction background script may be prone to race conditions if interactions occur concurrently. Consider synchronizing access to shared state or ensuring proper cleanup of UI elements.
• Reason this comment was not posted:
  Comment was on unchanged code.

9. scripts/fletchit.lic:1507

• Draft comment:
  Typo: "untill" should be "until" in the warning message.
• Reason this comment was not posted:
  Comment was on unchanged code.

10. scripts/fletchit.lic:1511

• Draft comment:
  Typo: "untill" should be "until" in the warning message.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. scripts/fletchit.lic:1517

• Draft comment:
  Typo: "untill" should be "until" in the warning message.
• Reason this comment was not posted:
  Comment was on unchanged code.

12. scripts/fletchit.lic:1295

• Draft comment:
  Typo: "finshed arrows" should be "finished arrows" in the GTK tooltip description.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a straightforward typo correction in a tooltip string. The comment is correct - "finshed" should be "finished". This is user-facing text that appears in the GUI setup window, so fixing it would improve the user experience. The comment is actionable, clear, and identifies a real issue. According to the rules, comments that suggest code quality improvements are good if they are actionable and clear. This qualifies as it's a simple spelling fix. Could this be considered too minor or obvious? The rules say "Do NOT make comments that are obvious or unimportant." A typo in a tooltip might be considered minor. However, it's user-facing text and the fix is non-obvious (you have to spot the typo). The rules also say comments should be about code changes - I need to verify this line was actually changed in the diff. Looking at the diff, line 1295 is in the new file and appears to be part of code that was reorganized/refactored. The old version had similar tooltip text. This appears to be existing code that was moved/reformatted rather than newly written. The rules state "Do NOT comment on anything that would be obviously caught by the build" - but typos in strings won't be caught by the build. The rules also say "Only comment if there is clearly a code change required" - fixing a typo is a code change. However, the most important rule is "you must see STRONG EVIDENCE that the comment is correct in order to keep it" and "If you are not sure, or if understanding the comment requires more context (such as seeing other files or parts of the codebase), you should delete the comment." I can clearly see the typo exists, so there is strong evidence. But I should check if this is about a change in the diff. The typo exists and is verifiable on line 1295. However, checking the diff more carefully, this appears to be in code that existed before (the old version had similar tooltip text with the same typo). Since the comment should only be about changes made by the diff, and this typo appears to have existed before this PR, the comment should be deleted. The rule is clear: "Do NOT comment unless there is clearly a code change required" and "If the comment is about unchanged code, this should be False, and the comment should be deleted."

13. scripts/fletchit.lic:1297

• Draft comment:
  Typos: "what ever" should be "whatever" and "get comand" should be "get command" in the tooltip description.
• Reason this comment was not posted:
  Comment was on unchanged code.

14. scripts/fletchit.lic:1507

• Draft comment:
  Typo: "you supplies" should be "your supplies" in the warning message.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_agpyiEGnAOxShrUF

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

[Copilot]

Copilot wasn't able to review any files in this pull request.

---

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

[AndreasWelch]

Feedback is welcome. I am keeping in draft as I continue to test.

There's almost certainly more work that could be done on this, breaking out functions and such, which I may or may not do before finalizing here.