Refactor ./container/dangerzone.py #167
Conversation
|
Hi @gmarmstrong. Would you have the time to address this review feedback I gave a while ago? (It should be pretty minor) |
|
Thanks! Now I think you're only missing catching that |
|
Should be good now! Do double-check that it works, of course. |
|
Awesome! I am just going to do some final testing tomorrow but all looks great. Thanks! I may do a rebase prior to merging, OK? |
Go for it! |
deeplow
force-pushed
the
refactor-converter
branch
from
cdfef25 to
097fe7d
Compare
|
Rebased, tested on a Linux and Windows system and made lint happy. |
|
Nice, giving it a look as well. |
| run_command( | ||
| [ | ||
| "gm", | ||
| "convert", | ||
| "-size", | ||
| f"{width}x{height}", | ||
| "-depth", | ||
| "8", | ||
| f"rgb:{rgb_filename}", | ||
| f"pdf:{pdf_filename}", | ||
| ], | ||
| error_message=f"Page {page}/{num_pages} conversion to PDF failed", | ||
| timeout_message=f"Error converting RGB to PDF, convert timed out after {DEFAULT_TIMEOUT} seconds", | ||
| ) |
There was a problem hiding this comment.
We define the same gm command twice, regardless of whether we want OCR or not. Shouldn't we factor it out?
There was a problem hiding this comment.
Almost the same, but not exactly the same. The first is from RGB to PNG, the second is from RGB to PDF.
There was a problem hiding this comment.
Oh, you're right, thanks for pointing it out. Still, I think we can do the factoring out (e.g., gm_convert_to(), and add the target file type and filename as arguments to that command).
There was a problem hiding this comment.
I see your point, although gm convert is also used (very differently) in the document_to_pixels function, so that function should be named gm_convert_pixels_to. Alternatively, it could be a nested function, but that could be counterproductive to readability.
Since the duplicate code wasn't introduced in this PR, I think it would be better to address it in a later PR. This one introduces the run_command function, which shrank the two gm convert steps considerably already.
There was a problem hiding this comment.
Sure, makes sense. We're good to go then!
@deeplow do you want to merge this, or should I go ahead?
|
Thanks @apyrgio, I'll take a look this weekend. |
Standardize calls to subprocess.run to shrink file by about 100 lines
Co-authored-by: deeplow <[email protected]>
Co-authored-by: deeplow <[email protected]>
Co-authored-by: deeplow <[email protected]>
Co-authored-by: deeplow <[email protected]>
deeplow
force-pushed
the
refactor-converter
branch
from
2566445 to
2085405
Compare
|
Rebased and merged. Thanks again @gmarmstrong and sorry that this took so long. |
Cleans up the container script by consolidating all calls to
subprocess.runto a single function. Shrinkscontainer/dangerzone.pyfrom 541 lines to 397 total.Everything (except
__main__itself) has been refactored to raise exceptions instead of returning an integer. Intended to cause no noticeable changes to program behavior.