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

Resolves #17, Resolves #18, Support --destination-dir --out-file #33

Merged
merged 3 commits into from
May 5, 2019

Conversation

ggrossetie
Copy link
Owner

@ggrossetie ggrossetie commented Dec 9, 2018

ping @matklad 😉

So the following is supported:

-o foo/bar/baz/book.pdf

Will create foo, bar and baz directories (if they don't exist).

-o -

Will output the result (pdf content) to the console

-D foo/bar/baz -o book.pdf

Will create foo, bar and baz directories (if they don't exist).

-o book.pdf

Will create a file named book.pdf

Please note that for now we do not remove the temporary HTML file (to ease development/debugging).
Also the temporary HTML file is always created next to the input file. So you don't have to update the relative paths when you want to output the PDF file somewhere else.

lib/converter.js Outdated
} finally {
if (outputToStdout && await existsFile(outputFile)) {
console.log(await readFile(outputFile, 'utf-8'))
await removeFile(outputFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: does JavaScript have some kind of try-with-resources (as in Java) / with (as in Python) analogue? As written, there's a theoretical possibility that readFile throws, and outFile doesn't get removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it seems like we should close browser in finally as well?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Curious: does JavaScript have some kind of try-with-resources (as in Java) / with (as in Python) analogue?

As far as I know no 😞

As written, there's a theoretical possibility that readFile throws, and outFile doesn't get removed.

Yes you're right, I should probably add another try/catch block.

Also, it seems like we should close browser in finally as well?

I'm using await browser.close() in the finally block but since readFile can throw an exception...

lib/converter.js Outdated
await removeFile(outputFile)
}
// QUESTION: should we remove the temporary HTML file ?
// It useful when building a new layout but the converter should only output a PDF file.
Copy link
Contributor

Choose a reason for hiding this comment

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

I use temp html file for live preview: I open it in browser with autoreload. I think I can use something similar with PDF.

Perhaps we need to delete html by default, but then, for html-preview specifically, have a flag --generate-only-html?

OTOH, if PDF generation is not significanly slower then html generation, perhaps I can adjust my workflow to preview PDFs direclty?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I use temp html file for live preview: I open it in browser with autoreload. I think I can use something similar with PDF.

Indeed but the major drawback is that it can be harder to debug/prototype.
I'm using the dev tools a lot when I create a new layout.

Perhaps we need to delete html by default, but then, for html-preview specifically, have a flag --generate-only-html?

Why not...
The HTML page is an intermediate step so maybe --keep-html ?

OTOH, if PDF generation is not significantly slower then html generation, perhaps I can adjust my workflow to preview PDFs directly?

It depends on the size or your document but the PDF generation is relatively fast.

@matklad
Copy link
Contributor

matklad commented Dec 15, 2018

-o ''

Hm, shoudn't this be -o -?

@ggrossetie
Copy link
Owner Author

-o ''

Hm, shoudn't this be -o -?

I need to try, maybe if it's supported by https:/yargs/yargs but if it is then yes we should use -o - to be consistent with the Asciidoctor CLI.

@ggrossetie
Copy link
Owner Author

Thanks for your review @matklad (greatly appreciated). I will update my pull request 👍

@ggrossetie
Copy link
Owner Author

-o ''

Hm, shoudn't this be -o -?

@matklad It's working with -o - 🎉

@ggrossetie
Copy link
Owner Author

I've added two functions safelyReadFile and safelyRemoveFile to read and remove files in the finally block.

@ggrossetie ggrossetie force-pushed the mkdirs-destination-to-file-support branch from 4e73b26 to 9cd48c5 Compare January 24, 2019 13:31
@ggrossetie
Copy link
Owner Author

@matklad This pull request reverts #38 because the temporary file is now written by Asciidoctor.js (using a simple fs.writeFile)

@ggrossetie ggrossetie force-pushed the mkdirs-destination-to-file-support branch from 9cd48c5 to b2b9d81 Compare May 2, 2019 15:16
@ggrossetie ggrossetie force-pushed the mkdirs-destination-to-file-support branch from b2b9d81 to f991416 Compare May 2, 2019 15:27
@ggrossetie ggrossetie merged commit 4d65abf into master May 5, 2019
@ggrossetie ggrossetie deleted the mkdirs-destination-to-file-support branch October 22, 2019 11:01
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.

2 participants