feat(gameobj-data.xml): add new jarable type

[mrhoribu]

adds new jarable type and populates it with shells and gems by default
updates existing lib/migration helpers to accommodate stuffing existing type data into another
fix rspec workflow to retain gameobj-data.xml artifact for quick download/testing

---

Important

Add new jarable type to gameobj-data.xml, update migration logic, and modify RSpec workflow to retain XML artifact.

• Behavior:
  • Add new jarable type in type_data/migrations/66_jarable.rb with default shells and gems.
  • Update migration logic in lib/migration/change-set.rb and lib/migration/migrator.rb to support jarable type.
  • Modify RSpec workflow in .github/workflows/rspec_tests.yml to retain gameobj-data.xml artifact.
• Tests:
  • Update spec/gameobj-data/core_spec.rb and spec/gameobj-data/gem_spec.rb to include jarable type.
• Misc:
  • Minor formatting and whitespace changes in lib/lich/gameobj.rb and lib/migration/convert.rb.

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 791806a in 3 minutes and 30 seconds. Click for details.

• Reviewed 1023 lines of code in 14 files
• Skipped 0 files when reviewing.
• Skipped posting 11 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. .github/workflows/rspec_tests.yml:32

• Draft comment:
  Consider using a stable release tag (e.g. v2.x) for the upload-artifact action instead of a commit hash, unless a specific commit is required.
• 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 suggesting a style/pattern change, but the existing code in the same file consistently uses commit hashes for all GitHub actions (with version comments). This suggests the team has deliberately chosen to pin actions to specific commit hashes, which is actually a security best practice to prevent supply chain attacks. The comment is asking for a change that would make this line inconsistent with the rest of the file. This appears to be a preference/style comment that doesn't recognize the existing pattern. The comment also uses "Consider" and "unless a specific commit is required" which makes it speculative and not a clear code change requirement. Perhaps the team recently decided to switch from tags to commit hashes for security reasons, and this comment is actually pointing out an inconsistency with a new standard. Or maybe the commit hash being used is incorrect or doesn't correspond to a stable version. Looking at the full file, ALL other actions use commit hashes consistently (lines 20, 21, 26), so this is clearly the established pattern in this codebase. The comment would make the code inconsistent with the existing style. The comment is also phrased as a suggestion ("Consider") rather than identifying a clear bug or issue. This comment should be deleted. It suggests changing the code to use a release tag instead of a commit hash, but the entire file consistently uses commit hashes for all GitHub actions, indicating this is the team's deliberate security practice. The comment would make the code inconsistent with the existing pattern and is phrased as a suggestion rather than identifying a clear issue.

2. lib/migration/change-set.rb:21

• Draft comment:
  The ChangeSet.run method now requires an extra 'tables' parameter. Consider adding documentation or comments to clarify its purpose.
• 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 is about a change (the new tables parameter), so it passes that test. However, I need to evaluate if it's useful. The rules state "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." This comment is suggesting adding documentation/comments, which is somewhat subjective. The purpose of the tables parameter is actually fairly clear from the context - it's used in the new helper methods get_table and copy_rules_from. The inline comment "# Add this line" at line 32 and "# Add helper methods" at line 39 suggest this is new functionality. The comment is asking for documentation, but it's not pointing out a bug or a clear code issue - it's a style/documentation suggestion. According to the rules, I should be looking for "STRONG EVIDENCE that the comment is correct" and comments should require "clearly a code change required." Asking for documentation is not a required code change - it's a nice-to-have. The comment could be considered actionable (add a comment to the parameter), and documentation is generally good practice. The purpose might not be immediately obvious to someone unfamiliar with the codebase, and adding a brief comment could improve code maintainability. While documentation is good practice, the rules explicitly state to only comment when there is "clearly a code change required" and to avoid "obvious or unimportant" comments. Documentation suggestions are typically not required changes - they're nice-to-haves. The purpose of the tables parameter can be inferred from the new helper methods that use it. This falls into the category of a suggestion rather than a required fix. This comment should be deleted. It's suggesting documentation as a nice-to-have improvement rather than identifying a required code change or bug. The purpose of the tables parameter is reasonably clear from the context and the new helper methods that use it.

3. lib/migration/migration.rb:41

• Draft comment:
  The to_xml method accepts a '_tables' parameter that isn’t used; it relies on the instance variable @tables instead. Consider either using the parameter or removing it for clarity.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. type_data/migrations/66_jarable.rb:55

• Draft comment:
  When using copy_rules_from('gem', ...), ensure that the source table 'gem' exists. This dependency should be documented or safeguarded.
• 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 dependency exists and to document or safeguard it. This falls under the rule of not asking the author to ensure behavior or document dependencies, which is not allowed.

5. lib/migration/table.rb:37

• Draft comment:
  Typographical note: The error message currently states "allowed game-obj.xml keys:". Given that the commit message and other comments refer to "gameobj-data.xml", it might be clearer and more consistent to update this string to "allowed gameobj-data.xml keys:".
• Reason this comment was not posted:
  Comment was on unchanged code.

6. lib/migration/table.rb:68

• Draft comment:
  Typographical error: The comment "the name of the table in gameob-data.xml" contains a typo. It should likely read "gameobj-data.xml" to match the naming convention used elsewhere.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. lib/migration/table.rb:81

• Draft comment:
  Typographical error: "ontop" should be written as two words, "on top", for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

8. lib/migration/table.rb:115

• Draft comment:
  Typo: "existance" should be spelled "existence".
• Reason this comment was not posted:
  Comment was on unchanged code.

9. type_data/migrations/66_jarable.rb:11

• Draft comment:
  Typo suggestion: In 'candystick tellin shell', consider correcting to 'candlestick telling shell' if that is the intended phrase.
• 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% "Candystick tellin" is a real shell name - tellins are a type of bivalve mollusk, and the candystick variety has distinctive striped coloring. The suggested correction "candlestick telling" doesn't make sense in the context of shell names. This appears to be a data migration file adding shell names to a database, and the names should be accurate representations of actual shells. The comment is suggesting an incorrect change that would replace a correct scientific/common name with nonsense. I might be wrong about "candystick tellin" being a real shell name - perhaps I'm making assumptions about marine biology terminology. The automated tool might have access to a dictionary or spell-checker that flagged this as unusual. Even if I'm uncertain about the exact taxonomy, "candlestick telling shell" makes no sense as a shell name, while "candystick tellin" follows the pattern of other shell names in the list (descriptive adjective + shell type). The comment is clearly incorrect in its suggestion. This comment should be deleted. It incorrectly suggests changing what appears to be a legitimate shell name ("candystick tellin") to nonsensical text ("candlestick telling"). The suggestion would introduce an error rather than fix one.

10. type_data/migrations/66_jarable.rb:14

• Draft comment:
  Typo suggestion: 'crown-of-Charl shell' may have a misspelling. Perhaps it should be 'crown-of-Charles shell'?
• 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 appears to be game data, likely from GemStone IV based on the "Solhaven Bay" reference and other fantasy-themed names. In GemStone IV, "Charl" is the name of a deity (the god of the sea). A "crown-of-Charl shell" would be a shell named after this deity, which makes perfect sense in context. The automated tool likely flagged this as a typo because it doesn't recognize "Charl" as a proper noun. This is a false positive - the comment is incorrect and should be deleted. I might be wrong about the GemStone IV context - perhaps this is from a different game or system. However, even if I'm wrong about the specific source, the comment is speculative ("may have a misspelling", "Perhaps it should be") rather than definitively identifying an error, which violates the rule against speculative comments. Even if I'm uncertain about the exact context, the comment is clearly speculative in nature ("may have", "Perhaps"), which violates the rules. The automated tool is guessing that this might be wrong without strong evidence. Given that this is data being intentionally added by the PR author, and the comment provides no strong evidence of an actual error, it should be deleted. This comment should be deleted. It's a speculative suggestion about a potential typo without strong evidence. The term "Charl" is likely a proper noun (a deity name in game lore), and the comment violates the rule against speculative comments that aren't definitively identifying issues.

11. type_data/migrations/66_jarable.rb:43

• Draft comment:
  Typo suggestion: The pattern in other entries uses a space rather than a hyphen in phrases like 'gryphon's wing shell'. Consider changing 'siren's-comb' to 'siren's comb' for consistency.
• 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 is making an assumption about consistency based on one other entry ("gryphon's wing shell"). However, there are many entries with hyphens in compound adjectives (black-spined, blue-banded, pink-banded, yellow-banded, ruby-lined, three-lined, tiger-striped, purple-cap, crown-of-Charl). The hyphen in "siren's-comb" could be intentional as it's a compound noun modifier. Without knowing the actual names of these shells in the game/domain this is for, it's speculative to say this is a typo. The comment is making an assumption about what the "correct" name should be based on limited pattern matching, but these could be actual proper names that should be preserved as-is. This seems like a speculative comment about consistency rather than a definite issue. However, the comment does point out a potential inconsistency between "siren's-comb" and "gryphon's wing" which are structurally similar (possessive + noun). If these are data entries that should follow consistent formatting rules, this could be a legitimate issue. The author might have made a typo or inconsistency. While there is a structural similarity between "siren's-comb" and "gryphon's wing", without domain knowledge about the actual names of these shells, we cannot definitively say this is wrong. These could be proper names from a game or specific domain where "siren's-comb" is the correct spelling. The comment is speculative about what the "correct" form should be, and doesn't provide strong evidence that this is actually an error rather than an intentional naming choice. This comment is speculative about consistency without strong evidence that "siren's-comb" is incorrect. It could be the actual proper name in the domain. The comment should be deleted as it doesn't meet the threshold of being a definite issue.

Workflow ID: wflow_TrngSKkplsth9Z35

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