adworse/iguvium

Problem with double splat operator in Ruby 3.0.0

Closed this issue · 2 comments

Environment

Ruby version: 3.0.0p0 (2020-12-25 revision 95aff21468) [x86_64-linux]
IRB version: 1.4.1 (2021-12-25)
Ruby Package Manager version: 3.2.29
Ghostscript version: Ghostscript 9.50 (2019-10-15)
OS: Ubuntu 20.04.3 LTS

Desired behaviour

Execute example code without any bugs

Current behaviour

Argument error when executing Iguvium.read('file.pdf')

Workaround

I've found that removing the double splatter operator(**) from Page initializer fixed it for me. Everything else seemed to be normal when debugging, including the Ghostscript path.

Steps to reproduce bug

# Using IRB or via script
require 'iguvium'
Iguvium.read('file.pdf')

Page.rb before fix

  def initialize(page, path, **opts)
    @opts = opts
    @reader_page = page
    @path = path
  end

Page.rb after fix

  def initialize(page, path, opts)
    @opts = opts
    @reader_page = page
    @path = path
  end

Results before fix

iguvium

Results after fix

iguvium3

Since I'm not sure this solution is the best, I've opened this issue in case someone wants to discuss it.

PS: after fixing the first bug, another one appeared when executing extract_tables!. I fixed it in the same way. If this fix is approved, then it should probably be applied to all the lines where double splat is being used.

EDIT: after finding the real fix, I've searched for similar method calls throughout the project, and it seems that the CLI is suffering from the same problem in exe/iguvium:33:

CLI Bug

Screenshot from 2022-12-01 14-06-18

After fix

Screenshot from 2022-12-01 14-06-34

It seems that my first fix is not correct. The real reason why this error occurs is due to the method call, and not to the method signature. The more appropriate fix would be, for example, to change iguvium.rb line 76, and every other method call that sends a hash as the last keyword arguments.

Fix example in iguvium.rb:76

Original code:
PDF::Reader.new(path, opts).pages.map { |page| Page.new(page, path, opts) }
Fixed code:
PDF::Reader.new(path, opts).pages.map { |page| Page.new(page, path, **opts) }

PS: there's already a pull request to solve this case. its #5

@jlucartc thank you so much! Sorry for the delay, merged #5 and fixed it in 0.9.2 release