Open
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Contributor
Author
|
this is ready for review/merge whenever appropriate |
Contributor
Author
|
@ellipsis remove previous comments and review again |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to ee31ec8 in 2 minutes and 9 seconds. Click for details.
- Reviewed
177lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7draft 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/go2.lic:835
- Draft comment:
Default initialization for the 'unhide' option is set (e.g. 'CharSettings["unhide"] = false if ...') which is good. Consider reviewing consistency with similar boolean defaults. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. scripts/go2.lic:1193
- Draft comment:
The regex for '--unhide' command-line option correctly converts input to a boolean using the lookup (setting_value). Consider using a direct boolean check later for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. scripts/go2.lic:1954
- Draft comment:
At runtime, the new 'unhide' option is integrated: if true and player is hidden then 'fput("unhide")' is issued, else if false typeahead is set to 0. Consider replacing 'is_a?(TrueClass)' checks with a simple boolean condition for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. scripts/go2.lic:858
- Draft comment:
The help output now includes a clear '--unhide=<on|off>' description, which is well integrated with the new option. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. scripts/go2.lic:18
- Draft comment:
Typo: In the changelog entry for version 2.3.0, the sentence 'Add "unhide" option, when true unhides when hidden. when false(default) and hidden, sets typeahead to 0' has a few issues: - The sentence after the period ('when false(default) and hidden, sets typeahead to 0') should start with a capital letter. - There should be a space in 'false(default)' (e.g., 'false (default)'). Please correct these typographical errors. - 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 pure formatting/style in a changelog entry. It's not about code logic or functionality. The changes suggested are very minor and don't impact understanding. The changelog is primarily for documentation purposes and these small formatting issues don't reduce its clarity. Could inconsistent changelog formatting make it harder to read and parse the changelog entries over time? Could these small issues compound if left uncorrected? While consistency is good, these are extremely minor formatting issues in a changelog that is already quite readable. The meaning is clear regardless of capitalization or spacing. This level of nitpicking on documentation formatting is not valuable. The comment should be deleted as it focuses on trivial formatting issues in documentation that don't impact functionality or clarity.
6. scripts/go2.lic:924
- Draft comment:
Typo alert: The substring "vaalor" appears in the regex and possibly in the variable name (e.g.,change_map_vaalor_shortcut). Did you mean "valor" instead? Please verify if this spelling is intentional. - Reason this comment was not posted:
Comment was on unchanged code.
7. scripts/go2.lic:1194
- Draft comment:
Typographical Review: It appears that the assignment for the unhide option usessetting_value[$1.downcase]. Verify ifsetting_valueis the correct variable name, or if it should be something else (e.g., a mapping likeunhide_options), to ensure consistency with the surrounding code. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_nlUsfAgfr88LNc82
You can customize 
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 7ab74be in 2 minutes and 24 seconds. Click for details.
- Reviewed
177lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7draft 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/go2.lic:857
- Draft comment:
Good default initialization for the 'unhide' option. Consider adding a brief comment explaining that when the player is hidden and 'unhide' is false, the script disables typeahead (by setting it to 0). - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
2. scripts/go2.lic:2142
- Draft comment:
The new unhide handling uses 'is_a?(TrueClass)' and 'is_a?(FalseClass)'. Consider using direct boolean comparison (e.g. 'if setting_unhide') for clarity and consistency. Also, add a comment explaining that if hidden and unhide is false, typeahead is disabled. - 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 code style and clarity. The suggestion to useif setting_unhideinstead ofis_a?(TrueClass)is a valid Ruby idiom - direct boolean checks are more idiomatic than type checking. However, I need to consider if there's a reason for the explicit type checking. Looking at the broader context, I see the codebase has been changed from.class ==checks to.is_a?checks (line 19 in changelog says "Change class checks to is_a? checks"). So the author deliberately choseis_a?over direct boolean comparison. The explicit type checking might be intentional to handle nil values differently. The second part about adding a comment is somewhat subjective - the code is fairly self-explanatory (if hidden and unhide is false, set typeahead to 0). This feels like a style preference rather than a clear code issue. The author just changed the codebase from.class ==to.is_a?checks as part of this PR, so they may have intentionally chosen this pattern for consistency or to handle nil values explicitly. The suggestion to use direct boolean comparison might conflict with their design intent. The request for a comment is subjective - the code logic is relatively clear from reading it. While the comment has merit from a Ruby idiom perspective, it conflicts with the deliberate change the author made in this PR (moving to is_a? checks). The explicit type checking may be intentional to distinguish between nil, true, and false values. The comment about adding explanatory text is subjective and the code is reasonably self-documenting. This is more of a style preference than a clear issue. This comment should be deleted. It suggests a style change that conflicts with the deliberate pattern the author established in this PR (using is_a? checks), and the request for additional comments is subjective when the code logic is already fairly clear.
3. scripts/go2.lic:1295
- Draft comment:
The echo message for the 'unhide' option is clear. For consistency with other boolean option messages, consider standardizing the output format (e.g. using 'on'/ 'off' instead of 'will'/'will not'). - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
4. scripts/go2.lic:18
- Draft comment:
Typographical note: Consider adding a space between "false" and "(default)" (i.e., change "false(default)" to "false (default)"). Also, since there's a period before "when false...", it might read better with the sentence starting with a capital letter (e.g., "When false (default) and hidden, sets typeahead to 0"). - 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 comment about formatting/style in a changelog entry. The suggestions are: 1) Add a space between "false" and "(default)", and 2) Capitalize "When" at the start of the second sentence. These are very minor stylistic suggestions. The rules say "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." Changelog formatting is not code, and these suggestions are extremely minor - the changelog is perfectly readable as-is. This seems like the kind of pedantic comment that would be annoying to receive as a PR author. It's not fixing a bug, improving logic, or addressing a real issue - it's just nitpicking punctuation and capitalization in a changelog. Could there be a style guide for this project that requires specific changelog formatting? Maybe consistency in changelog entries is important for automated parsing or documentation generation? Even if there were a style guide, this is such a minor formatting issue that it's unlikely to be worth commenting on. Looking at other changelog entries in the file (lines 20-103), I can see various formatting styles are used - some sentences start with capital letters, some don't, and there's no strict consistency. This suggests there's no rigid style guide being enforced. The comment is too pedantic. This comment should be deleted. It's making very minor stylistic suggestions about changelog formatting that don't improve code quality or fix any real issues. It's the kind of pedantic comment that wastes the PR author's time.
5. scripts/go2.lic:454
- Draft comment:
Typo: "vaalor shortcut" might be a misspelling. Did you intend to use "valor shortcut"? - 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.
6. scripts/go2.lic:1215
- Draft comment:
Typo alert: In this new code block, 'setting_value' is used on line 1215. It looks like a potential typo, as the variable name doesn't match the naming pattern (e.g., 'setting_unhide'). Please verify if 'setting_value' is intended or if it should be corrected to a consistent variable name. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. scripts/go2.lic:1783
- Draft comment:
Typo: Consider revising the error message for grammatical correctness. Instead of "neither a Proc or a String", it should likely be "neither a Proc nor a String". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_jAMa4ytsjDx9oCCY
You can customize
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes introduce a new "unhide" setting option to the Go2 script, enabling control over whether hidden targets are automatically unhidden before movement. Additionally, movement logic type-checking is refactored to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/go2.lic`:
- Line 396: The setup flow is accidentally clearing CharSettings['unhide']
because load_go2_settings adds :unhide but Setup prunes keys not listed in
@@categories so settings[:unhide] can be absent and you end up writing nil; fix
this by either adding :unhide to the class-level @@categories array so Setup
will preserve it, or change the write-back that assigns CharSettings['unhide']
(where you use settings[:unhide]) to use a fallback like settings.fetch(:unhide,
CharSettings['unhide']) so the existing value is retained when the key is
missing.
- Around line 2142-2146: The current branch only honors the command-line
variable setting_unhide but ignores saved CharSettings['unhide'], so change the
logic to fallback to the saved setting when setting_unhide is nil/unset: read
CharSettings['unhide'] into setting_unhide (or use it as a fallback) before the
conditional that checks setting_unhide.is_a?(TrueClass)/FalseClass; then keep
the existing actions (calling fput("unhide") when true and setting
setting_typeahead = 0 when false) and preserve the checkhidden guard.
| :rogue_password => UserVars.rogue_password, | ||
| :delay => CharSettings['delay'], | ||
| :typeahead => CharSettings['typeahead'], | ||
| :unhide => CharSettings['unhide'], |
Contributor
There was a problem hiding this comment.
setup flow can clear unhide by saving nil.
load_go2_settings now includes :unhide, but Setup prunes keys that are not declared in @@categories. Since unhide is not declared there, settings[:unhide] can be missing and Line 450 writes nil back to CharSettings['unhide'].
💡 Proposed fix
@@
@@categories = {
general: {
@@
typeahead: { default: 0 },
+ unhide: { default: false },
delay: { default: 0 },
@@
- CharSettings['unhide'] = settings[:unhide]
+ CharSettings['unhide'] = settings.fetch(:unhide, CharSettings['unhide'])Also applies to: 450-450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/go2.lic` at line 396, The setup flow is accidentally clearing
CharSettings['unhide'] because load_go2_settings adds :unhide but Setup prunes
keys not listed in @@categories so settings[:unhide] can be absent and you end
up writing nil; fix this by either adding :unhide to the class-level
@@categories array so Setup will preserve it, or change the write-back that
assigns CharSettings['unhide'] (where you use settings[:unhide]) to use a
fallback like settings.fetch(:unhide, CharSettings['unhide']) so the existing
value is retained when the key is missing.
Comment on lines
+2142
to
+2146
| if setting_unhide.is_a?(TrueClass) && checkhidden | ||
| fput("unhide") | ||
| elsif setting_unhide.is_a?(FalseClass) && checkhidden | ||
| setting_typeahead = 0 | ||
| end |
Contributor
There was a problem hiding this comment.
Saved unhide setting is ignored during normal travel.
At Line 2142, behavior depends on setting_unhide, but that variable is only set when --unhide=... is passed on the command line. If users saved CharSettings['unhide'], it is not applied here.
💡 Proposed fix
@@
if setting_delay.nil?
setting_delay = CharSettings['delay']
end
+
+ if setting_unhide.nil?
+ setting_unhide = CharSettings['unhide']
+ end
@@
- if setting_unhide.is_a?(TrueClass) && checkhidden
- fput("unhide")
- elsif setting_unhide.is_a?(FalseClass) && checkhidden
- setting_typeahead = 0
- end
+ if checkhidden
+ if setting_unhide
+ fput('unhide')
+ else
+ setting_typeahead = 0
+ end
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/go2.lic` around lines 2142 - 2146, The current branch only honors the
command-line variable setting_unhide but ignores saved CharSettings['unhide'],
so change the logic to fallback to the saved setting when setting_unhide is
nil/unset: read CharSettings['unhide'] into setting_unhide (or use it as a
fallback) before the conditional that checks
setting_unhide.is_a?(TrueClass)/FalseClass; then keep the existing actions
(calling fput("unhide") when true and setting setting_typeahead = 0 when false)
and preserve the checkhidden guard.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Important
Adds
unhideoption togo2.licfor unhiding when hidden and updates class checks to useis_a?.unhideoption ingo2.licto unhide when hidden; defaults to false, setting typeahead to 0 if hidden.is_a?instead ofclass ==ingo2.lic.:unhidetoCharSettingsingo2.lic.CharSettings['unhide']to false if nil ingo2.lic.go2.lic.--unhide=<on|off>option to help output ingo2.lic.This description was created by
for 7ab74be. You can customize this summary. It will automatically update as commits are pushed.