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

Optionally track progress #804

Merged
merged 12 commits into from
Mar 16, 2019

Conversation

corincerami
Copy link
Contributor

This is my attempt to bring #278 to completion. I'll be testing the code out more thoroughly tomorrow, but I wanted to open this to see if it matched what you were looking for.

@corincerami
Copy link
Contributor Author

Rubocop is reporting the following offenses:

lib/wicked_pdf.rb:33:1: C: Class has too many lines. [313/300]
class WickedPdf ...
^^^^^^^^^^^^^^^
lib/wicked_pdf.rb:123:9: W: Do not suppress exceptions.
        rescue Errno::EIO # child process is terminated, this is expected behaviour
        ^^^^^^^^^^^^^^^^^

The rescue is expected, so I'm not sure what to do about that offense. As for the too many lines offense, how would you prefer that be dealt with?

@unixmonkey
Copy link
Collaborator

@chrisccerami

The class length one is a tough one. We've bumped into a large-class threshold.
The best solution I think might be to move invoke_with_progress out into a separate module, something like this:

lib/wicked_pdf/progress.rb

class WickedPdf
  module Progress
    def invoke_with_progress(command, options)
      # method body
    end
  end
end

lib/wicked_pdf.rb

require 'wicked_pdf/progress'

class WickedPdf
  include Progress

  DEFAULT_BINARY_VERSION = Gem::Version.new('0.9.9')

I've been meaning to break this class up for awhile, and this might be a good start.

If you have trouble with this approach though, I wouldn't be opposed to you bumping the value in .rubocop-todo.yml to 320.

For the rescue, RuboCop only complains if it doesn't have a body.

Fixing that could be as simple as adding a message like:

rescue Errno::EIO # child process is terminated, this is expected behaviour
  puts 'Child process terminated' # or maybe a better message
ensure
  ::Process.wait pid

Or by having Rubocop ignore it in your file, or on that line:
.rubocop.yml

Lint/HandleExceptions:
  Exclude:
    - 'lib/wicked_pdf/progress.rb'
rescue Errno::EIO # rubocop:disable Lint/HandleExceptions
  # child process is terminated, this is expected behaviour
ensure
  ::Process.wait pid

Thank you for trying to tackle this. May I ask why exactly you want this though? Do you surface this to the user with the progress proc? If so, I'd also love an example for the README.

@corincerami
Copy link
Contributor Author

Yes, we're planning on using WickedPdf to generate PDF reports from inside of Resque jobs and want to surface progress to the user. I can add a snippet to the README showing the basic idea.

@corincerami
Copy link
Contributor Author

@unixmonkey ok should be good to go I think.

@unixmonkey
Copy link
Collaborator

Looks great! Could you also add the progress: option to the Advanced Usage with All Available Options section?

Since this takes a callable instead of the normal string, number, or boolean, perhaps something extra simple like this:

progress: proc { |output| puts output } # proc called when console output changes

@corincerami
Copy link
Contributor Author

How's that @unixmonkey ?

@unixmonkey
Copy link
Collaborator

@chrisccerami Great. I'm going to pull this version into an app and kick the tires a bit before merging, but I think this is good to go. 👍

@corincerami
Copy link
Contributor Author

Great, thanks.

@unixmonkey unixmonkey merged commit 35babcf into mileszs:master Mar 16, 2019
@unixmonkey
Copy link
Collaborator

This is pretty dang neat. I was able to track what page was being rendered with this:

def test_progress
  respond_to do |format|
    format.pdf do
      page_number = 0
      render pdf: 'mypdf', progress: -> (out) {
        match = out.match(/Page (\d+) of/)
        if match && match[1].to_i > page_number
          page_number += 1
          puts page_number
        end
      }
    end
  end
end

Thank you so much for the contribution, and putting up with my requests!

@corincerami corincerami deleted the optionally_track_progress branch March 16, 2019 16:47
@corincerami
Copy link
Contributor Author

No problem, happy to help. Currently our app has this code monkeypatched in, but once a new release is put out, we can test it out in our Gemfile.

@unixmonkey
Copy link
Collaborator

@chrisccerami I've just released version 1.2.0, which includes @Figedi and yours contributions. Thanks for pushing this forward!

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.

3 participants