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

Allow multiple pdf files to be converted #40

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

litemikx
Copy link

  • Remove "or" assignment on Pdf2Img.prototype.setOptions() to avoid overwriting options object whenever
    Pdf2Img.prototype.convert is called.
  • Added function Pdf2Img.prototype.convertList() to convert list of pdf
    files in array. 1st argument is the array, 2nd argument is the "options"
    object.

Fitra Aditya and others added 21 commits May 15, 2017 00:01
Fixed using gm as node_module to identify pages
It works fine on Windows (checked). Put link how required GraphicsMagick with Ghostscript can be installed on Windows.
Update README.md. Add instruction for windows users.
- Remove or assign on options to avoid overwriting options whenever
Pdf2Img.prototype.convert is called.
- Added function Pdf2Img.prototype.convertList() to convert list of pdf
files in array. 1st argument is the array, 2nd argument is the "options"
object.
- Remove "or" assignment on Pdf2Img.prototype.setOptions() to avoid
overwriting options object whenever
Pdf2Img.prototype.convert is called.
- Added function Pdf2Img.prototype.convertList() to convert list of pdf
files in array. 1st argument is the array, 2nd argument is the "options"
object.
- Remove "or" assignment on Pdf2Img.prototype.setOptions() to avoid
overwriting options object whenever
Pdf2Img.prototype.convert is called.
- Added function Pdf2Img.prototype.convertList() to convert list of pdf
files in array. 1st argument is the array, 2nd argument is the "options"
object.
@litemikx
Copy link
Author

Sorry about that, this is my first time contributing to a project and probably got confused with some of the steps. Wanted to share the changes I made that allows converting multiple pdfs in an array. Any feedback is appreciated, not sure though why CI build is failing. Thanks!

@@ -16,11 +16,11 @@ var options = {
var Pdf2Img = function() {};

Pdf2Img.prototype.setOptions = function(opts) {
options.type = opts.type || options.type;
Copy link
Contributor

@elhigu elhigu Dec 30, 2017

Choose a reason for hiding this comment

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

This doesn't seem right, now if options has already value and it is not given in opts it will be overridden with undefined.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I made another commit that should handle assignments on the options variable.

options.density = opts.density || options.density;
options.outputdir = opts.outputdir || options.outputdir;
options.outputname = opts.outputname || options.outputname;
options.type = opts.type != null || opts.type != 'undefined' ? opts.type : options.type;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is broken in so many levels that I don't know where to start. Why have you changed these line at all? AFAIK the original implementation was correct.

Copy link
Author

Choose a reason for hiding this comment

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

Needed to edit the lib to be used in converting list of PDF files, so far it's working on my end. Most likely my code is not clean or should have better error handling. Added a new function at the bottom that handles an array of PDF where "options" can be passed. Oh well, will need to re-visit this next time.

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.

4 participants