Skip to content
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.

Sign up for GitHub

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

Remove unused InputMethod#initialize #635

Merged
merged 2 commits into from
Aug 7, 2023
Merged

Remove unused InputMethod#initialize #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01f615a
Remove unused InputMethod#initialize
st0012 Jul 8, 2023
4e26b31
Remove unused FileInputMethod#file_name
st0012 Jul 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions lib/irb/input-method.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,7 @@
require 'reline'

module IRB
STDIN_FILE_NAME = "(line)" # :nodoc:
class InputMethod

# Creates a new input method object
def initialize(file = STDIN_FILE_NAME)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a search on GitHub, InputMethod.new was found to be used in two repositories.
I believe it would be better to show a deprecation warning before removal.

https://github.com/search?q=%22+InputMethod.new%22+language%3ARuby&type=code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's thoughtful to think about external dependants when dropping things 👍 But in this case I think we should not be blocked by them because:

  1. irb_remote hasn't received any update for 5 years and it already passes incorrect number of arguments to InputMethod.new. So it's already broken.
  2. In irb-power_assert, it's used to build an IRB environment for testing incorrectly. I say incorrectly because InputMethod is an abstract class. So in practice it should have not been initialised directly. If the author switch to StdioInputMethod instead, that may be better and more stable.

But the root problem is that we and previous maintainers don't define what's official APIs, what's not, and ways/principles to achieve what they need without using private APIs.

My current categorisation is:

So IMO InputMethod, or even its child classes like RelineInputMethod...etc., should be considered private. Otherwise, our work on refactoring IRB would be seriously affected.

What are your thoughts? @tompng @hasumikin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I understand, and it does not seem to be a problem to delete InputMethod.new.
However, I think we should carefully migrate the other InputMethods since they are widely used.
Not all of the code is IRB-related, but there seems to be a lot there. (Not sure if they have been active recently)
https://github.com/search?q=InputMethod.new+language%3ARuby+&type=code

Copy link
Member

@tompng tompng Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the refactor looks good. InputMethod should not have FileInputMethod specific attribute.

My opinion for public and private are:

Sub classes: IMO should be private forever. There are alternatives.

RelineInputMethod # Only used in ruby/irb and it's forked repository
StdioInputMethod # Some repository using this is too old. last update: 12 years ago

# Alternative of IRB::Irb.new(workspace, FileInputMethod.new(file_name))
IRB::Irb.new(workspace, file_name)

# Alternative of IRB::Irb.new(workspace, ReadlineInputMethod.new)
IRB.conf[:USE_MULTILINE] = false

InputMethod: Maybe we can make it public in the future. But now, there is no way to set output method. It's hard to implement something like remote IRB using socket because users can only change input. So the use case of custom input method is very limited.

IRB::Irb#initialize had ouput_method but it was unused/unimplemented and removed in 53f7769

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your consideration, I was facing CI failures with these changes, but this discussion helps me a lot.
I just replaced s/InputMethod/IRB::StdioInputMethod/ for now.

Feel free to leave care about irb-power-assert anymore, I will catch up IRB changes in #673 if needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kachick Thank you for the feedback and sorry for breaking your project's CI.
After taking a closer look, I think the test setup actually doesn't need to specify an input method at all. So I opened kachick/irb-power_assert#114 and I hope it will simplify its maintenance in the long run 🙂

@file_name = file
end
# The file name of this input method, usually given during initialization.
attr_reader :file_name

# The irb prompt associated with this input method
attr_accessor :prompt

Expand Down Expand Up @@ -56,7 +47,6 @@ def inspect
class StdioInputMethod < InputMethod
# Creates a new input method object
def initialize
super
@line_no = 0
@line = []
@stdin = IO.open(STDIN.to_i, :external_encoding => IRB.conf[:LC_MESSAGES].encoding, :internal_encoding => "-")
Expand Down Expand Up @@ -130,12 +120,9 @@ def open(file, &block)

# Creates a new input method object
def initialize(file)
super
@io = file.is_a?(IO) ? file : File.open(file)
@external_encoding = @io.external_encoding
end
# The file name of this input method, usually given during initialization.
attr_reader :file_name

# Whether the end of this input method has been reached, returns +true+ if
# there is no more data to read.
Expand Down Expand Up @@ -183,7 +170,6 @@ def initialize
if Readline.respond_to?(:encoding_system_needs)
IRB.__send__(:set_encoding, Readline.encoding_system_needs.name, override: false)
end
super

@line_no = 0
@line = []
Expand Down Expand Up @@ -261,7 +247,6 @@ class RelineInputMethod < InputMethod
# Creates a new input method object using Reline
def initialize
IRB.__send__(:set_encoding, Reline.encoding_system_needs.name, override: false)
super

@line_no = 0
@line = []
Expand Down
1 change: 0 additions & 1 deletion test/irb/helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ class TestInputMethod < ::IRB::InputMethod
attr_reader :list, :line_no

def initialize(list = [])
super("test")
@line_no = 0
@list = list
end
Expand Down