wubzz / pdf-merge

Merge multiple PDF Files into a single PDF document
127 stars 32 forks source link

:sparkles: - pdf merge now works on arrays of length 1 #23

Closed JLuzz closed 6 years ago

JLuzz commented 6 years ago

I think you should reconsider throwing an error in the case the array is equal to length 1

Although it makes no difference just merging one document, if the case ever does come up it is better to just let it run instead of throwing an error

For example, the code I am working with the input to pdf merge is based on the size of the users document. If their document only has 1 page pdf-merge will throw an error

wubzz commented 6 years ago

I'm not sure I understand your use case given the description below

For example, the code I am working with the input to pdf merge is based on the size of the users document. If their document only has 1 page pdf-merge will throw an error

So can it be multiple documents (PDF files) with 1 or more pages but the user only has one document? Or is it a single document (PDF file) with 1 or more pages? The latter would not need pdf-merge at all, so I'm assuming its the former..

Even if it is the former, it feels very counterintuitive to run pdftk with a single document - for what purpose? Your app would be running external apps for no reason at all. Waste of time and resources. One can't merge a single bare entity with nothingness.

For handling a single PDF file as input to pdf-merge I see no other option than to either throw an error like now or by returning the original document in the expected output format. Anything else will just result in wasting time and resources.

JLuzz commented 6 years ago

It is the latter. You are correct in stating that a merge with one entity with nothingness resolves the single entity, so it is wasteful to use system resources. However, I don't think that the case of 1 pdf should be treated as an error.

pdf-merge would be more flexible if it accepts the case of 1 and returns the original document or the expected output format, like you said. I would be happy to write up a PR that handles this case without firing up pdftk

wubzz commented 6 years ago

You're welcome to do so. Any solution that does not involve calling pdftk is ok with me.

JLuzz commented 6 years ago

Along with adding the return statement I also changed the error cases to one-liners, let me know what you think. I can change them back if you don't like them

JLuzz commented 6 years ago

Once I added the return statements, output became undefined for the files.length === 1 case, so I moved both options and output to be declared before that case occurs. Is there a better way to organize it?

wubzz commented 6 years ago

@JLuzz I much rather prefer keeping and using brackets like if(length > 1) {...} rather than if(length > 1) ....

What I meant in my previous comment was the following:

if(files.length === 1) {
    readFile(files[0])
    .then((buffer) => {
      return resolve(output(buffer));
    })
    .catch(reject);

      return;
}

Adding a return after the promise chain prevents the rest of the function being executed, which in turn means you won't need the new if statement if(files.length > 1) below which is also always going to be true if we reach that point of the code.

JLuzz commented 6 years ago

Thank you for the clarification

wubzz commented 6 years ago

Thanks, looks good now