The source location of generic commands was incorrect #170
Conversation
It should be the source location of the method of the appropriate subclass.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the source location reporting for generic commands to properly reflect the source location of the method implementation in the appropriate mode subclass, rather than the location where the generic command wrapper is defined.
Key changes:
- Modified
Commandfrom a Struct to a Data class with an additionalsource_location_procfield that can dynamically compute the source location - Updated
define_generic_commandto provide a lambda that retrieves the actual method's source location from the current buffer's mode - Changed help display to use the new
cmd.source_locationmethod instead of directly accessingcmd.block.source_location
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/textbringer/commands.rb | Converts Command from Struct to Data.define, adds source_location method and source_location_proc parameter to support dynamic source location resolution |
| lib/textbringer/mode.rb | Updates define_generic_command to pass a lambda that retrieves the source location from the actual mode method implementation |
| lib/textbringer/commands/help.rb | Updates command_help to use the new cmd.source_location method instead of directly accessing the block's source location |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| command_name = (name.to_s + "_command").intern | ||
| define_command(command_name, **options) do |*args| | ||
| define_command(command_name, | ||
| source_location_proc: -> { Buffer.current.mode.method(name).source_location rescue nil }, |
There was a problem hiding this comment.
The bare rescue here catches all exceptions, including system-level exceptions like SystemExit and SignalException. Consider rescuing specific exceptions (e.g., rescue NoMethodError, NameError) to avoid suppressing unexpected errors that should propagate.
| source_location_proc: -> { Buffer.current.mode.method(name).source_location rescue nil }, | |
| source_location_proc: -> { Buffer.current.mode.method(name).source_location rescue NameError, NoMethodError; nil }, |
There was a problem hiding this comment.
The rescue modifier doesn't catch SystemExit because such a system-level exception doesn't inherit StandardError.
It should be the source location of the method of the appropriate subclass.