feat(efamgo2.lic): v1.0.0 initial fork of famgo2.lic

[mrhoribu]

Important

Introduces efamgo2.lic script for navigating familiars or eyes to rooms, with new classes for navigation and error handling.

• New Script:
  • Adds efamgo2.lic as a fork of famgo2.lic.
  • Supports navigation of familiars or eyes to specified room numbers.
  • Initial release version 1.0.0.
• Classes and Functions:
  • Companion: Determines type and locates current room.
  • RoomResolver: Resolves room arguments to Room objects.
  • Navigator: Calculates and navigates paths between rooms.
  • Controller: Manages script execution and navigation process.
• Error Handling:
  • Introduces NavigationError, RoomNotFoundError, PathfindingError, and UnknownLocationError for specific error scenarios.
• Features:
  • Supports room identification by UID (u#####) and room ID.
  • Handles navigation through string and procedural waypoints.

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

[mrhoribu]

Improves upon #1943 and also supports UID being shown and used when FLAG is enabled vs title/description/exits lookup

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to d9cc588 in 3 minutes and 10 seconds. Click for details.

• Reviewed 390 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/efamgo2.lic:60

• Draft comment:
  Potential hang if 'At yourself?' is never returned; consider adding timeout/error handling.
• 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 raises a valid concern about a potential infinite loop if the expected response never arrives. However, I need to consider: 1) This appears to be a scripting environment (Lich) for a game where get and fput are standard commands for interacting with game output. 2) The pattern line = get until line == 'condition' is likely a common idiom in this scripting environment. 3) The comment is speculative ("Potential hang if...") rather than pointing to a definite bug. 4) Without knowing the behavior of the get method in this environment, I can't be certain this is actually a problem. 5) The rules state "Do NOT make speculative comments, such as 'If X, then Y is an issue'. Only comment if it is definitely an issue." This comment starts with "Potential" which makes it speculative. The comment might be valid if this is indeed a real risk in the Lich scripting environment. Perhaps the original famgo2.lic script (which this is a fork of) uses the same pattern without issues, suggesting this is standard practice. I might be too quick to dismiss a legitimate robustness concern. While the concern about infinite loops is generally valid in programming, the rules explicitly state not to make speculative comments. The comment uses "Potential" which indicates speculation. Additionally, this is a fork of an existing script (famgo2.lic), and if this pattern was problematic, it would likely have been addressed in the original. The scripting environment likely has its own conventions and safeguards. This comment should be deleted because it is speculative ("Potential hang if...") rather than pointing to a definite issue. The rules explicitly prohibit speculative comments. Without strong evidence that this is actually a problem in the Lich scripting environment, and given this is a fork of an existing script, the comment doesn't meet the threshold for keeping.

2. scripts/efamgo2.lic:223

• Draft comment:
  Avoid using global variable $step2_path for caching paths; encapsulate this state within Navigator or a dedicated class.
• 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/refactoring suggestion, not a bug or required change. The comment suggests encapsulating state better, which is generally good advice. However, looking at the rules, I need to consider: (1) This is a new file, so all code is "changed", (2) The global variables appear to be intentionally used for cross-script communication (the naming convention suggests this), (3) This is a refactoring suggestion, which the rules say are "good" but "only if they are actionable and clear", (4) The suggestion is somewhat actionable but may break compatibility with the existing script ecosystem that expects these globals. The use of globals here seems deliberate for interoperability with other scripts in the Lich scripting environment. I might be missing that this global variable usage is actually a bug or creates real problems. The comment could be highlighting a legitimate architectural issue that would cause problems. Also, just because it's a new file doesn't mean poor patterns should be accepted - maybe this is important enough to fix before merging. While the critique is valid that new code should follow best practices, the global variables appear to be part of an established pattern in this scripting ecosystem (evidenced by the naming conventions like $go2_use_seeking and $step2_path which suggest cross-script communication). The comment is a refactoring suggestion without demonstrating that the current approach causes actual problems. According to the rules, I should only keep comments that have "STRONG EVIDENCE" of being correct and necessary. This is more of a stylistic preference than a required fix. This comment should be deleted. It's a code quality refactoring suggestion without demonstrating that the current approach is broken or causes issues. The global variables appear to be intentionally used for cross-script communication in this ecosystem, and the comment doesn't provide strong evidence that this needs to be changed.

3. scripts/efamgo2.lic:348

• Draft comment:
  Avoid using the global variable $go2_use_seeking; consider managing this state within the Controller to reduce side effects.
• 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/refactoring suggestion about using global variables. The comment is on line 348 which is part of a new file being added. The rules state that "Comments that suggest code quality refactors are good! But only if they are actionable and clear." However, I need to consider: 1) This appears to be a fork of an existing script (famgo2.lic) and may be intentionally using globals for compatibility with the broader ecosystem, 2) The use of $go2_use_seeking and $step2_path might be part of a convention in this scripting environment (Lich scripts for games), 3) Without understanding the broader context of how these scripts interact, it's hard to say if removing globals would break compatibility, 4) The comment is somewhat generic and doesn't provide specific guidance on HOW to refactor this. I might be missing that these global variables are part of an established API or convention in the Lich scripting environment. The script appears to be interacting with other systems (Map, XMLData, Char, etc.) and the globals might be intentional for cross-script communication. The comment doesn't demonstrate strong evidence that this is actually a problem. While code quality suggestions can be valuable, this comment lacks strong evidence that the global variable usage is problematic in this context. The script is a fork and may need to maintain compatibility with existing conventions. The suggestion is also not very actionable - it doesn't explain what specific problems the global causes or provide a clear alternative approach. This comment should be deleted. It's a generic code quality suggestion without strong evidence of a problem, and it may not account for the conventions of the scripting environment this code operates in. The comment is not actionable enough and doesn't clearly demonstrate why this change is necessary.

4. scripts/efamgo2.lic:299

• Draft comment:
  The 'respond' method is used without a clear definition; ensure it exists or replace it with a proper logging/notification function.
• 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% This is a new file (the diff shows it's being added), so all the code is new. The comment is about a change (since everything is new). However, the comment appears to be based on unfamiliarity with the Lich scripting framework. In Lich scripts, respond is a standard built-in method just like echo, fput, get, etc. The comment assumes respond doesn't exist and needs to be replaced, but this is incorrect. The choice between respond and echo is semantic - respond outputs to the game window while echo outputs to the script window. For a warning message, either could be appropriate depending on where the author wants it displayed. This is not a bug or error. I might be wrong about the Lich framework having a respond method. Perhaps in this specific version or context, respond isn't available. However, the presence of other Lich-specific methods throughout the code strongly suggests this is a standard Lich environment where respond should be available. Given that the code uses multiple Lich-specific methods (fput, get, before_dying, XMLData, Map, Room, Script.current, Lich::Messaging), and these are all standard Lich framework methods, it's highly likely that respond is also available as a standard method. The comment appears to be based on unfamiliarity with the framework rather than identifying a real issue. This is not actionable feedback. This comment should be deleted. It appears to be based on unfamiliarity with the Lich scripting framework where respond is a standard built-in method. The suggestion to replace it with echo is not a bug fix but rather a stylistic preference about where warnings should be displayed, which is not appropriate for an automated review comment.

5. scripts/efamgo2.lic:105

• Draft comment:
  When building a regex from XMLData.familiar_room_exits, consider escaping each element to avoid unexpected regex behavior.
• 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 defensive programming. The question is: can exit names in this game contain regex special characters? Without knowing the domain, I can't be 100% certain. However, room exits in text-based games could potentially have names like "door (locked)", "path [hidden]", or other special characters. The suggestion is reasonable defensive programming. But the rules say I should only keep comments with STRONG EVIDENCE of being correct. I don't have strong evidence that exit names will contain regex metacharacters - this is more of a "best practice" suggestion. The code is working with game data that might be well-formed and predictable. Also, looking at line 127-129, the author DOES use Regexp.escape when building a description regex, which shows they're aware of this technique but chose not to use it here - possibly because they know the exit data is safe. The author explicitly uses Regexp.escape elsewhere in the code (line 127), showing they understand this pattern. Their choice not to use it for exits might be intentional, suggesting they know the exit data format is safe. Without concrete evidence that exit names contain regex metacharacters, this is speculative. While the author does use Regexp.escape elsewhere, that doesn't necessarily mean they consciously decided against it here - it could be an oversight. However, the key issue is that I have no strong evidence this is actually a problem. The comment is a "consider" suggestion about potential issues, which makes it speculative rather than definitive. This is a speculative code quality suggestion without strong evidence of an actual bug. The comment uses "consider" language and discusses "unexpected regex behavior" that might happen, not something that definitely will happen. Per the rules, speculative comments should be deleted.

6. scripts/efamgo2.lic:284

• Draft comment:
  Prefer using a local match variable instead of relying on global variables like $2 in regex captures for clarity and thread safety.
• Reason this comment was not posted:
  Marked as duplicate.

7. scripts/efamgo2.lic:10

• Draft comment:
  Typo detected: "what to tell you pet" on line 10 likely should be "what to tell your pet".
• 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 straightforward typo fix suggestion. The comment correctly identifies that "you pet" should be "your pet". However, I need to consider the rules: the rules say "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." A typo in a comment block is not a code change - it's a documentation/text change. The typo doesn't affect functionality. While it's nice to fix typos, this falls into the category of being minor/unimportant and not requiring a code change. The rules emphasize only keeping comments that require clear code changes. While this is a legitimate typo, the rules explicitly state to only comment when there's clearly a code change required. A typo in a comment block is documentation, not code. This might be considered too minor or obvious to warrant a comment, especially since it doesn't affect functionality. The rules are clear that comments should be about code changes that are required, not documentation improvements. Even though this is a valid typo, it's in a comment block (documentation) and doesn't affect the actual functionality of the code. This falls under "obvious or unimportant" changes. This comment should be deleted. While it correctly identifies a typo, it's in a documentation comment block and doesn't represent a required code change. The rules emphasize only keeping comments for clear code changes, not minor documentation fixes.

Workflow ID: wflow_6BqPxtBIciog2ed7

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