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

Batch mode and command line tests #81

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

anko
Copy link
Collaborator

@anko anko commented Jan 30, 2016

A breaking change: I changed the -o/--out flags to expect a directory. Specifying a file path down to the name is nonsensical when dealing with multiple files. The files are processed with as much parallelism as I could manage with async.

The CLI getting bigger was making me uncomfortable, so I added some initial unit tests for it. They only cover the really important parts for now, but it's some peace of mind.

Closes #47.

Fixes alanshaw#47.

Changed the -o/--out option to specify a directory instead of a full
path for a file.
Made tests lint the executable too, and changed its style to conform.
These are the most important "obvious" functions that the command line
needs to be able to do, so might as well have an automated check that
they're still there.
@ckreon
Copy link

ckreon commented Apr 11, 2016

Is there a reason this hasn't been merged?

@alanshaw
Copy link
Owner

Incredible work. I'm sorry I never got round to reviewing this. I'm really stoked that you've added tests and in principle I'm happy for the CLI to be able to process and output multiple files.

The small issue I can see is here is that the existing functionality of specifying multiple files and having them combined into one PDF has gone. The original thought behind this was that if you were creating a thesis or a book or something you might want to have the chapters in separate markdown files and combine them together with the tool. I'd like to retain this functionality if possible, and I think we can with a little inspection of the arguments.

I'd propose:

multiple inputs + single file path output (ends in .pdf) = combine into one
multiple inputs + directory path output = process individually

...and for single file input:

single file input + single file path output (ends in .pdf) = process individually (specify name)
single file input + directory path output = process individually (automatic name)

What do you think @anko?

@anko
Copy link
Collaborator Author

anko commented Feb 18, 2017

@alanshaw

The original thought behind this was that if you were creating a thesis or a book or something you might want to have the chapters in separate markdown files and combine them together with the tool. I'd like to retain this functionality if possible, and I think we can with a little inspection of the arguments.

Some thoughts:

I'm at least initially against this. It is trivial to cat files together as necessary (or type/copy on Windows) before passing them to markdown-pdf. Adding more implicit rules for what certain types of argument combinations mean makes it harder to document, understand, and maintain.

After all this time to think, I'm doubting my own PR here too. It's easy to use other programs to run multiple markdown-pdf processes in parallel, like with & or GNU Parallel. This PR's functionality-changes only remove the overhead of having multiple Node.js processes, which is very little. However, this also enables a future optimisation of rendering multiple documents using the same PhantomJS process, which would be a huge performance improvement.

Maybe we should cherry-pick the cleanups and tests from this and leave the rest to merge later, together with a hypothetical only-one-PhantomJS-process optimisation patch. Then at least we don't break API for (almost) nothing.

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.

3 participants