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.

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

restore support for IRB <= v1.13.0 #358

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

sealocal
Copy link
Contributor

@sealocal sealocal commented Dec 3, 2024

#339 dropped support of IRB older than v1.13.0, which introduced #351

This change restores support for all versions of the irb gem

IRB 1.6.3

require 'bundler/inline'

gemfile do
  ruby '2.7.8'
  source 'https://rubygems.org'
  gem 'irb', '1.6.3'
  gem 'iruby', '0.8.0'
end

puts "The IRB gem is at version #{IRB::VERSION}"
puts "The IRuby gem is at version #{IRuby::VERSION}"

@plainbackend = IRuby::PlainBackend.new
@plainbackend.eval('puts "Hello from plainbackend!"', false)

# => iruby-0.8.0/lib/iruby/backend.rb:60:in `eval': undefined method `build_statement' for #<IRB::Irb:0x000000015bb0ce38> (NoMethodError)
...

gemfile do
  ruby '2.7.8'
  source 'https://rubygems.org'
  gem 'irb', '1.12.0'
  gem 'iruby', '0.8.0'
end

# ...
# => irb-1.12.0/lib/irb/workspace.rb:117:in `eval': no implicit conversion of IRB::Statement::Expression into String (TypeError)
...

gemfile do
  ruby '2.7.8'
  source 'https://rubygems.org'
  gem 'irb', '1.13.0'
  gem 'iruby', '0.8.0'
end

# ...
# => Hello from plainbackend!

@kojix2
Copy link
Member

kojix2 commented Dec 3, 2024

Hello!
Good pull request.
However, I'm concerned about the fact that you're creating an instance of IRB::RegexpCompletor every time.

def initialize
  # ommited
  @completor = defined?(IRB::RegexpCompletor) ? IRB::RegexpCompletor.new : nil
end

def complete(code)
  if @completor
    @completor.completion_candidates('', code, '', bind: @workspace.binding)
  else
    IRB::InputCompletor::CompletionProc.call(code)
  end
end

Wouldn't this be better?

Note: I don't have any professional Ruby/Rails development experience, so I'm not necessarily familiar with the best practices for Ruby web development.

# preposing and postposing never used, so they are empty, pass only target as code
@completor.completion_candidates('', code, '', bind: @workspace.binding)
if defined? IRB::RegexpCompletor # IRB::VERSION >= 1.8.2
completor = IRB::RegexpCompletor.new
Copy link
Member

Choose a reason for hiding this comment

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

It seems that an instance of IRB::RegexpCompletor is created each time. This seems inefficient, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, agree! Added e07b7eb

@sealocal sealocal force-pushed the restore-old-irb-support branch from 5efff04 to e07b7eb Compare December 3, 2024 07:45
@sealocal
Copy link
Contributor Author

sealocal commented Dec 3, 2024

Hello! Good pull request. However, I'm concerned about the fact that you're creating an instance of IRB::RegexpCompletor every time.

def initialize
  # ommited
  @completor = defined?(IRB::RegexpCompletor) ? IRB::RegexpCompletor.new : nil
end

def complete(code)
  if @completor
    @completor.completion_candidates('', code, '', bind: @workspace.binding)
  else
    IRB::InputCompletor::CompletionProc.call(code)
  end
end

Wouldn't this be better?

Note: I don't have any professional Ruby/Rails development experience, so I'm not necessarily familiar with the best practices for Ruby web development.

Ah, agree - that was an oversight and changes more than I intended. I've restored the attribute!

@kojix2 kojix2 merged commit 36720b7 into SciRuby:master Dec 3, 2024
7 of 8 checks passed
@kojix2
Copy link
Member

kojix2 commented Dec 3, 2024

Thank you. Please send another pull request if you find anything else.

@sealocal sealocal deleted the restore-old-irb-support branch December 3, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants