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

Encapsulate binary path and version handling #816

Merged
merged 3 commits into from
Oct 12, 2020

Conversation

adzap
Copy link
Contributor

@adzap adzap commented Mar 20, 2019

Cleans up main class and allows refactoring of path handling.

Binary path discovery can be complex it seems and this offers a path to improved handling or overriding if necessary.

Long term goal of combining extracted option parsing and this to allow a command class to combine and execute and return result.

I think these combined PRs provide a path to a well factored and a cleaner handling gem's functions.

adzap added 3 commits March 20, 2019 20:18
Cleans up main class and allows refactoring of path handling
Leave binary_version read method in the public api

This reverts commit 751d375.
@adzap
Copy link
Contributor Author

adzap commented Mar 22, 2019

Same problem with the build again.

@unixmonkey
Copy link
Collaborator

Rebuild passed :)

@adzap
Copy link
Contributor Author

adzap commented Mar 22, 2019

thanks!

The overall ambition, to be able to combine an option parser object with a binary object consumed by a command object which handles the actual cli execution. This is coordinated by a document object that is home to pdf_from_string, pdf_from_url etc. methods.

Decoupled, testable, customisable.

@adzap
Copy link
Contributor Author

adzap commented Mar 22, 2019

This redesign would support things like #768 much more easily and cleanly.

@unixmonkey
Copy link
Collaborator

I had to resolve some minor merge conflicts related to xvfb, but this is now merged.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants