search

wubzz / pdf-merge

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

Fix errors when filenames contains spaces. #15

Closed 0x3333 closed 7 years ago

0x3333 commented 7 years ago

Fix issue #14 .

wubzz commented 7 years ago

Thanks! Published.

wubzz commented 7 years ago

@0x3333 Reverting this since it actually broke merges. See #17

I can't reproduce #14, so I guess I merged this prematurely.

0x3333 commented 7 years ago

Are you sure about it? The PR is just to quote the file path, which is mandatory in case of paths with spaces. I'll take a look at it, but I'm using the version with the quotes and it is working, despite the version without it. I'll investigate the #17. Thanks!

wubzz commented 7 years ago

Yeah I was confused as well. I tried it locally after this PR and it threw the errors from #17. Presumably PDFtk for some reason considered the "'s to be part of the file name, which is weird.

0x3333 commented 7 years ago

For sure PDFtk is not the problem, it just receive the parameters, I believe that the problem is the distinction between windows vs others. I'll check this, couz the right thing to do is escape, but let me check it more deeply. I'll let you know my discoveries.

timyhac commented 6 years ago

Right now, this library uses shell-escape, but it doesn't handle windows OS correctly - https://github.com/xxorax/node-shell-escape/issues/9

0x3333 commented 6 years ago

In my case, I changed locally. But the PR caused some problems, I had no time to debug. But for sure, it is broken.

wubzz commented 6 years ago

Yeah I'm not sure why this caused problems tbh, can't quite remember. I've re-added this particular change and added a test for files with spaces in them.

FYI; Yes this library uses shell-escape, but it's never called when run on windows.

Will publish once I get home.