Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds introspection utilities for Reline keybindings and an IRB command to display them. The PR introduces functionality to inspect and format Reline keybindings in a human-readable way, along with an interactive IRB command interface.
Changes:
- Added introspection module to query and format Reline keybindings with special key symbols
- Created an IRB command to display keybindings in a formatted table
- Added comprehensive test coverage for the introspection functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| lib/reline_pac/introspection.rb | Implements keymap inspection and key sequence formatting with support for control keys, arrow keys, and special characters |
| lib/reline_pac/irb_commands.rb | Defines an IRB command class to display keybindings in a formatted output |
| spec/reline_pac/introspection_spec.rb | Provides test coverage for the introspection module's keymap functionality |
| lib/reline_pac.rb | Integrates the new introspection module and conditionally loads IRB commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case byte | ||
| when 27 | ||
| '⎋' | ||
| when 0..31 # Control キー |
There was a problem hiding this comment.
The comment on line 78 contains Japanese text ('Control キー' meaning 'Control key'). For consistency with the rest of the codebase where English is used, this comment should be written in English.
| when 0..31 # Control キー | |
| when 0..31 # Control key |
| it 'includes known keybindings' do | ||
| result = described_class.keymap | ||
| # Control-A should map to ed_move_to_beg | ||
| expect(result).to include('⌃A' => :ed_move_to_beg) |
There was a problem hiding this comment.
The test expects Control-A (⌃A) to always map to ed_move_to_beg, but this assumption depends on the default Reline configuration and may vary across different environments or Reline versions. This could make the test brittle. Consider either mocking the keymap data or testing the formatting logic independently of specific keybinding values.
| it 'includes known keybindings' do | |
| result = described_class.keymap | |
| # Control-A should map to ed_move_to_beg | |
| expect(result).to include('⌃A' => :ed_move_to_beg) | |
| it 'includes and formats movement keybindings when present' do | |
| result = described_class.keymap | |
| # If ed_move_to_beg is present, its key should be formatted as a Control key | |
| key_for_move_to_beg = result.key(:ed_move_to_beg) | |
| if key_for_move_to_beg | |
| expect(key_for_move_to_beg).to include('⌃') | |
| end |
| # Load commands only if IRB is available | ||
| require_relative 'reline_pac/irb_commands' if defined?(IRB) |
There was a problem hiding this comment.
The conditional require 'require_relative 'reline_pac/irb_commands' if defined?(IRB)' may not work as intended in all scenarios. The file itself requires 'irb' at the top (line 3 of irb_commands.rb), which means if IRB is not available when this line executes, it will raise a LoadError. The conditional should either wrap the require statement in the irb_commands.rb file or use a rescue block here to handle cases where IRB is not installed.
| # Load commands only if IRB is available | |
| require_relative 'reline_pac/irb_commands' if defined?(IRB) | |
| # Load commands only if IRB is available, and fail gracefully if IRB cannot be loaded | |
| begin | |
| require_relative 'reline_pac/irb_commands' if defined?(IRB) | |
| rescue LoadError | |
| # IRB is not installed or could not be loaded; skip IRB-specific commands | |
| end |
|
|
||
| def execute(_arg) | ||
| bindings = RelinePac::Introspection.keymap | ||
| max_key_length = bindings.keys.map(&:length).max |
There was a problem hiding this comment.
The execute method doesn't handle the case where bindings is empty or max_key_length is nil. If keymap returns an empty hash, bindings.keys.map(&:length).max will return nil, and calling ljust on a string with nil as an argument will raise a TypeError.
| max_key_length = bindings.keys.map(&:length).max | |
| max_key_length = bindings.keys.map(&:length).max || 0 |
| class Keybinds < ::IRB::Command::Base | ||
| category 'Context' | ||
| description 'Show Reline keybindings' | ||
|
|
||
| def execute(_arg) | ||
| bindings = RelinePac::Introspection.keymap | ||
| max_key_length = bindings.keys.map(&:length).max | ||
|
|
||
| bindings.each do |key, method| | ||
| puts "#{key.ljust(max_key_length + 2)} #{method}" | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The test cases don't cover the IrbCommands::Keybinds class, which means the execute method and command registration logic lacks test coverage. Since other modules in the repo have corresponding test files, this command should also have tests.
| config = Reline.core.config | ||
| bindings = {} | ||
|
|
||
| config.keymap.instance_variable_get(:@key_bindings).each do |key, method| |
There was a problem hiding this comment.
The code uses instance_variable_get to access Reline's internal @key_bindings, which is a brittle approach that depends on internal implementation details. If Reline changes its internal structure in future versions, this code will break. Consider checking if Reline provides a public API for accessing keybindings, or add error handling for when the instance variable doesn't exist.
| config.keymap.instance_variable_get(:@key_bindings).each do |key, method| | |
| keymap = config.keymap | |
| key_bindings = | |
| if keymap.respond_to?(:key_bindings) | |
| keymap.key_bindings | |
| elsif keymap.instance_variable_defined?(:@key_bindings) | |
| keymap.instance_variable_get(:@key_bindings) | |
| else | |
| {} | |
| end | |
| key_bindings.each do |key, method| |
| when "\e[F", "\eOF" then 'End' | ||
| when "\e[H", "\eOH" then 'Home' | ||
| when "\e[1~", "\e[7~" then 'Home' # rubocop:disable Lint/DuplicateBranch | ||
| when "\e[4~", "\e[8~" then 'End' # rubocop:disable Lint/DuplicateBranch |
There was a problem hiding this comment.
The rubocop:disable comments for Lint/DuplicateBranch on lines 42 and 43 are suppressing legitimate warnings. These case branches do have duplicate 'Home' and 'End' return values. The duplicate branches could be combined (e.g., when "\e[F", "\eOF", "\e[4~", "\e[8~" then 'End') to eliminate the need for these suppressions and improve maintainability.
| when "\e[F", "\eOF" then 'End' | |
| when "\e[H", "\eOH" then 'Home' | |
| when "\e[1~", "\e[7~" then 'Home' # rubocop:disable Lint/DuplicateBranch | |
| when "\e[4~", "\e[8~" then 'End' # rubocop:disable Lint/DuplicateBranch | |
| when "\e[F", "\eOF", "\e[4~", "\e[8~" then 'End' | |
| when "\e[H", "\eOH", "\e[1~", "\e[7~" then 'Home' |
| end | ||
| end | ||
|
|
||
| # IRB が読み込まれている場合のみ登録 |
There was a problem hiding this comment.
The comment on line 24 is in Japanese while all other code and comments in the file are in English. For consistency and to ensure all team members can understand the codebase, comments should be written in English.
| # IRB が読み込まれている場合のみ登録 | |
| # Register only if IRB has been loaded |
… display them