-
Notifications
You must be signed in to change notification settings - Fork 28
feat(all): login refactor for entry yaml build and account management #910
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
base: staging
Are you sure you want to change the base?
Conversation
Too explicit for rescue clause
…t in yaml_state_spec
…t in yaml_state_spec
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.
Caution
Changes requested ❌
Reviewed 4a69934 in 3 minutes and 14 seconds. Click for details.
- Reviewed
152
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
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. lib/util/login_helpers.rb:146
- Draft comment:
Ensure that char_name is not nil before invoking casecmp? to avoid a potential NoMethodError. - 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 method documentation clearly states char_name is required. The comment suggests defensive programming that would hide bugs rather than catch them. If char_name is nil, that's a programming error that should fail fast. The suggested change would make the code more confusing by implying char_name could be nil when it shouldn't be. Perhaps there are edge cases where char_name could be nil that I'm not seeing? The method does accept nil as a valid value according to some documentation comments. Looking at the full context, while char_name can be nil in the method signature, in this specific block we're already past the point where we'd handle nil char_name cases - this is specifically for exact matches where we want char_name to be present. The comment should be deleted. The suggested change would add unnecessary defensive programming that could hide bugs rather than catch them.
2. lib/util/login_helpers.rb:175
- Draft comment:
For consistent behavior, consider assigning the normalized game code (e.g. using game_code.to_s.upcase) to :_requested_game_code in the fallback block rather than the raw game_code. - 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% While normalizing would be more consistent with other parts of the code, the current implementation doesn't seem to cause any actual issues since case-insensitive comparisons are used where needed. The suggestion is more about code style than fixing a real problem. The code works correctly as is. The code might be slightly more maintainable if all game codes were normalized at the point of storage rather than handling case sensitivity at each point of use. However, the current approach of handling case sensitivity at points of comparison is also valid and consistent. The code already works reliably. While the suggestion would improve code style slightly, it's not addressing a real problem and the current implementation is perfectly functional.
Workflow ID: wflow_eJN8xZFFL1QQysgP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 1c8f8d5 in 1 minute and 17 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
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. lib/util/login_helpers.rb:283
- Draft comment:
Removed debugging print. Consider using a proper logging call if debugging is needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_b0xXvo8vmEYykwxc
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Updated three instances of improper msg format remaining thanks to Tysong's sharp eyes.
spaces from GH web editor
Lich 5 GUI Login Refactoring Plan
Proposed Refactoring Approach
Address code organization issues, avoid code redundancy (DRY), correct existing legacy naming convention issues, implement state management, and provide appropriately documented code.
1. File Structure (Maintained)
We will keep the three-file structure:
gui_login
method2. Code Organization Improvements
In gui-login.rb:
gui_login
method to be more modularIn gui-saved-login.rb and gui-manual-login.rb:
3. DRY Improvements
Create shared utility modules for common functionality:
Lich::Common::GUI::Utilities
: Common GUI utilitiesLich::Common::GUI::Authentication
: Authentication logicLich::Common::GUI::Components
: GUI Component buildersLich::Common::GUI::State
: Common state codeExtract repeated code into reusable methods:
4. Naming Convention Standardization
5. Documentation Improvements
6. State Management Improvements
7. Implement account management in GUI
8. Implement entry.yml and retire entry.dat mechanisms
9. Implementation of accessibility features (supports Linux only)
10. Implementation of Favorites
11. Modify CLI login to use new YAML structure
Important
Refactor Lich 5 GUI login system for improved modularity, state management, and introduce YAML-based entry storage, account management, and accessibility features.
gui-login.rb
,gui-saved-login.rb
, andgui-manual-login.rb
for better modularity and error handling.yaml_state.rb
, replacingentry.dat
.account_manager.rb
andaccount_manager_ui.rb
.accessibility.rb
(Linux only).favorites_manager.rb
.main.rb
.gui-saved-login.rb
andgui-manual-login.rb
.utilities.rb
,components.rb
,theme_utils.rb
.account_manager_spec.rb
andauthentication_spec.rb
.yaml_state_spec.rb
.gui-login.rb
togui_login.rb
for naming consistency.This description was created by
for 1c8f8d5. You can customize this summary. It will automatically update as commits are pushed.