-
Notifications
You must be signed in to change notification settings - Fork 53
fix(eloot)v2.7.0 add looting open boxes on ground #2203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Deysh
wants to merge
13
commits into
elanthia-online:master
Choose a base branch
from
Deysh:eloot
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+309
−261
Conversation
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
Selling ingots if too overloaded to store updated find_trash to use trash verb general inventory code cleanup
Contributor
|
@ellipsis remove previous comments and review again |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to b4f4fd6 in 3 minutes and 48 seconds. Click for details.
- Reviewed
1095lines of code in1files - Skipped
0files when reviewing. - Skipped posting
25draft 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/eloot.lic:257
- Draft comment:
Initialized disk_full as a hash ({}), replacing a boolean. Ensure all references now expect a hash. - 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.
2. scripts/eloot.lic:619
- Draft comment:
New UI option 'use_disk_group' added—verify that the layout and default (false) meet expected behavior. - 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.
3. scripts/eloot.lic:2221
- Draft comment:
disk_usage now uses safe-navigation and Disk.mine; ensure Disk.mine reliably returns the correct disk object. - 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/eloot.lic:2377
- Draft comment:
Updated caller_method regex to match both backticks and single quotes; this improves compatibility with stack traces. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what was changed and why. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
5. scripts/eloot.lic:2715
- Draft comment:
find_trash now only uses the new trash verb and returns a GameObj immediately. Confirm that omitting prior fallback logic is acceptable. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that omitting prior fallback logic is acceptable. This falls under the rule of not asking the author to confirm their intention or to ensure behavior is intended. Therefore, this comment should be removed.
6. scripts/eloot.lic:3156
- Draft comment:
wait_for_disk now relies on Disk.mine and uses include? on status; verify that Disk.mine and the new status check work as intended. - 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.
7. scripts/eloot.lic:3488
- Draft comment:
single_drag_box now supports group disks: it checks use_disk OR use_disk_group and iterates Group.disks. Confirm that comparing item.type using '==' behaves as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to confirm the behavior of comparing item.type using '=='. This falls under the rule of not asking the author to confirm their intention or behavior. Therefore, this comment should be removed.
8. scripts/eloot.lic:3547
- Draft comment:
Manual hand-checks have been refactored to use the new in_hand? helper – this improves readability and consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only praises the refactoring without providing any actionable feedback or suggestions. It does not align with the rules for useful comments.
9. scripts/eloot.lic:3488
- Draft comment:
In the item storing failure block, ingots are automatically sold when in a locksmith pool. Verify that using exit (abrupt termination) is acceptable. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify if using 'exit' is acceptable, which is against the rules of asking for confirmation or verification. It doesn't provide a specific suggestion or point out a clear issue.
10. scripts/eloot.lic:5790
- Draft comment:
New handle_ingot method sells ingots and restores a note if present; ensure that switching rooms (using go2) doesn't disrupt gameplay unexpectedly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that switching rooms doesn't disrupt gameplay, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue with the code.
11. scripts/eloot.lic:4550
- Draft comment:
The new box_loot_ground method handles open boxes on the ground. Note that if coin collection fails due to overload, the script calls exit; consider if pausing might be preferable for graceful recovery. - 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 making a suggestion about error handling behavior - specifically suggesting that pausing might be better than exiting. However, this falls into several problematic categories: 1) It's speculative ("consider if pausing might be preferable") rather than pointing out a definite bug, 2) It's asking the author to consider/evaluate something rather than pointing to a clear code issue, 3) Looking at line 3267 in the diff, there's another similar pattern where the script exits when unable to pick up items due to overload, suggesting this is an intentional design pattern in the codebase. The comment doesn't provide strong evidence that exiting is wrong here - it's just suggesting an alternative approach without clear justification. The comment might have merit if there's a clear reason why pausing is better than exiting in this context. Perhaps other parts of the codebase handle similar situations with pausing, making this inconsistent. However, I can see at line 3267 that the script also exits when unable to handle additional load, suggesting this is the intended pattern. While there might be some precedent for pausing in the codebase (like at line 3478 where it pauses when failing to store items), the comment is still speculative and asks the author to "consider" rather than pointing to a definite issue. The existing pattern at line 3267 shows that exiting on overload is used elsewhere. Without strong evidence that this is wrong, this is just a subjective design suggestion. This comment should be deleted. It's a speculative suggestion asking the author to "consider" an alternative approach without providing clear evidence that the current approach is wrong. The codebase shows similar exit-on-overload patterns elsewhere, suggesting this is intentional design.
12. scripts/eloot.lic:6788
- Draft comment:
Group variables (disk_full and sacks_full) are reset and synced via reset_disk_full, ensuring consistency. Verify that this initialization properly reflects the group state. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify the behavior of the code, which violates the rule against asking for confirmation or verification of intended behavior. It does not provide a specific suggestion or point out a clear issue.
13. scripts/eloot.lic:1160
- Draft comment:
Typographical suggestion: In the tooltip message, consider using the possessive form "group members' disks" instead of "group members disks". - 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 minor grammatical suggestion about tooltip text. According to the rules, I should not comment on things that are obvious or unimportant. While the grammatical correction is technically correct, this is a very minor stylistic issue in a user-facing tooltip. The rules state "Do NOT make comments that are obvious or unimportant" and "Do NOT comment unless there is clearly a code change required." A missing apostrophe in a tooltip is not a critical code issue. Additionally, this appears to be about a line that was added in the diff (the use_disk_group tooltip), so it is about changes made. However, the comment is purely about grammar/style in a tooltip message, which is arguably unimportant and not a clear code change requirement. While grammatically correct, this could be considered a helpful improvement to user-facing text quality. Some teams do care about proper grammar in UI elements, and tooltips are user-facing. The suggestion is clear and actionable. Even though the grammar correction is valid, this falls under "obvious or unimportant" comments. The tooltip is perfectly understandable without the apostrophe, and this is not a code functionality issue. The rules explicitly state not to make comments that are obvious or unimportant, and a missing apostrophe in a tooltip qualifies as unimportant. This comment should be deleted. It's a minor grammatical suggestion about tooltip text that falls under "obvious or unimportant" comments according to the review rules. While technically correct, it doesn't represent a clear code change requirement.
14. scripts/eloot.lic:3136
- Draft comment:
Typo: The info message text has an unnecessary leading space. Consider removing the initial space in " Waiting for your disk to arrive" to improve consistency. - Reason this comment was not posted:
Comment was on unchanged code.
15. scripts/eloot.lic:3493
- Draft comment:
Typographical note: The comment says "Only wait if the characters disk exists". Consider changing it to "Only wait if the character's disk exists" to use the correct possessive form. - 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 rules state that comments should not be made unless there is clearly a code change required, and to avoid comments that are obvious or unimportant. This is a minor grammatical correction to a comment (not code), which doesn't affect functionality at all. While technically correct, this falls under "obvious or unimportant" - it's a very minor typo that doesn't impact understanding. The rules emphasize only keeping comments with STRONG EVIDENCE of being correct and necessary. A possessive apostrophe in a comment is not a meaningful code issue. One could argue that maintaining good grammar in comments improves code quality and professionalism, and that clear, correct documentation is important for maintainability. Some teams have strict style guides that include proper grammar in comments. While good grammar is nice to have, the rules explicitly state not to make comments that are "obvious or unimportant." A missing apostrophe in a comment doesn't affect code functionality, doesn't cause confusion about the meaning, and is extremely minor. This is the definition of an unimportant comment that doesn't warrant a PR review note. This comment should be deleted. It's a trivial grammatical correction to a code comment that has no impact on functionality or understanding. It violates the rule against making obvious or unimportant comments.
16. scripts/eloot.lic:3499
- Draft comment:
Minor typo: The comment reads "make sure its not full". It should be "make sure it's not full" (adding an apostrophe). - 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% According to the rules, I should NOT keep comments that are obvious or unimportant. A typo in a code comment (not in actual code) is very minor and doesn't affect functionality. The rules also state to only comment if there is clearly a code change required - fixing grammar in comments is not a code change that affects behavior. This is the kind of nitpicky, unimportant comment that should be removed. It's not wrong per se, but it's not useful enough to warrant keeping. One could argue that maintaining good grammar in comments improves code quality and readability, making the codebase more professional. Some teams do care about comment quality. While good grammar is nice to have, this is an extremely minor issue that doesn't affect code functionality or understanding. The rules explicitly say not to make comments that are "obvious or unimportant," and a missing apostrophe in a comment clearly falls into that category. This is the definition of a nitpicky comment that adds no real value. This comment should be deleted. It's pointing out a minor grammatical issue in a code comment that has no impact on functionality or code understanding. This violates the rule against making obvious or unimportant comments.
17. scripts/eloot.lic:3500
- Draft comment:
Minor typo: The comment says "skip the disk unless its present". Consider changing it to "skip the disk unless it's present" (adding an apostrophe). - 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 comment is about a typo in a code comment (not actual code). According to the rules, I should not comment on things that are "obvious or unimportant." A grammatical typo in a comment is very minor and doesn't affect functionality. The rules state "Do NOT make comments that are obvious or unimportant." While technically correct, fixing "its" to "it's" in a comment is extremely minor and not important for code functionality. This falls under the category of unimportant comments that should be removed. One could argue that maintaining good grammar in comments improves code quality and readability, making this a valid code quality suggestion. Some teams have strict standards about comment quality. While good grammar is nice to have, this is an extremely minor typo in a code comment that doesn't affect functionality or understanding. The rules explicitly say not to make obvious or unimportant comments, and a single apostrophe typo in a comment clearly falls into the "unimportant" category. This is not a meaningful code quality improvement. This comment should be deleted because it's pointing out an unimportant grammatical typo in a code comment that doesn't affect functionality or code understanding.
18. scripts/eloot.lic:4173
- Draft comment:
Typographical error: In the comment, consider changing "its" to "it's" for proper grammar. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a minor typographical error in a comment, which does not affect the functionality or logic of the code. It does not provide a meaningful suggestion or highlight a potential issue in the code itself.
19. scripts/eloot.lic:4332
- Draft comment:
Typographical: In the comment, "its a locker" should be "it's a locker". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typographical error in a comment, which is not critical to the functionality or logic of the code. It does not provide a suggestion for code improvement or address a potential issue in the code's behavior.
20. scripts/eloot.lic:4343
- Draft comment:
Typographical error: In the comment, "its a locker" should be "it's a locker". - Reason this comment was not posted:
Comment was on unchanged code.
21. scripts/eloot.lic:4404
- Draft comment:
Typo in comment: "its a locker" should be "it's a locker". - Reason this comment was not posted:
Comment was on unchanged code.
22. scripts/eloot.lic:4610
- Draft comment:
Typo: In the comment, "if its not gone it might be special - save it", consider changing "its" to "it's" for clarity. - 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 comment is about grammar in a code comment, not about actual code functionality. According to the rules, I should not keep comments that are obvious or unimportant. Grammar fixes in comments are typically low-priority and don't affect code behavior. The rules state "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 doesn't require a code change - the code works fine either way. This is a very minor stylistic issue that doesn't impact functionality. While good grammar is nice to have, this is an internal code comment that doesn't affect functionality. Some might argue that maintaining good grammar in comments improves code quality and readability, making it a valid suggestion. Even if good grammar improves readability slightly, this falls under "obvious or unimportant" comments that should be removed. The meaning of the comment is clear regardless of the grammatical error, and fixing it doesn't constitute a necessary code change. This is the type of minor nitpick that clutters PR reviews. This comment should be deleted. It's a minor grammatical suggestion about a code comment that doesn't affect functionality and falls under the category of "obvious or unimportant" comments that should not be made.
23. scripts/eloot.lic:5762
- Draft comment:
Typographical suggestion: In the log message, consider changing "its special" to "it's special" for clearer intent. - Reason this comment was not posted:
Comment was on unchanged code.
24. scripts/eloot.lic:6169
- Draft comment:
There's an extra leading space in the error message text. Consider removing the initial whitespace in " Failed to find a bell, keys, or chime on the table". - 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.
25. scripts/eloot.lic:6600
- Draft comment:
Typo: In the comment, "if its not gone" should be "if it's not gone". - 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 purely stylistic/grammatical comment about a code comment (not actual code). The rules state "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 doesn't require a code change - the code functions identically. This is an extremely minor grammatical issue that has no impact on functionality. The rules also say comments should be actionable, but fixing grammar in comments is not a meaningful code improvement. This falls under "obvious or unimportant" - it's a trivial grammar issue in a comment. One could argue that maintaining good grammar in comments improves code quality and readability, making it a valid code quality suggestion. Professional codebases often enforce proper grammar in comments. While good grammar is nice to have, the rules explicitly state not to make obvious or unimportant comments. A missing apostrophe in a comment is extremely minor and doesn't affect code functionality or understanding. This is the definition of an unimportant comment. This comment should be deleted. It's pointing out a trivial grammatical issue in a code comment that has no impact on functionality or code quality. This violates the rule against making obvious or unimportant comments.
Workflow ID: wflow_B3O4UVlNhOzG5ykW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Selling ingots if too overloaded to store
updated find_trash to use trash verb
general inventory code cleanup
Important
Enhancements to
eloot.licinclude ground looting, group disk support, improved inventory management, and trash handling updates.eloot.lic.box_loot_ground()to handle looting boxes on the ground.in_hand?()to check if an item is in hand, replacing multiple checks across the script.reset_disk_full()and updated disk usage logic indisk_usage().find_trash()to fully utilize the trash verb and simplify trashcan detection.dump_herbs_junk()to handle trash disposal more effectively.This description was created by
for b4f4fd6. You can customize this summary. It will automatically update as commits are pushed.