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

added outputFile option to override the generated filename #301

Merged
merged 3 commits into from
Jul 23, 2017

Conversation

tisoft
Copy link
Contributor

@tisoft tisoft commented Jul 17, 2017

The asciidoctor CLI allows to override the output filename with the -o, --out-file arguments. This change allows to do the same with the maven plugin.

This allows for example to include the project version in the filename:

<outputFile>my_lovely_documentation_${project.version}.pdf</outputFile>

@abelsromero
Copy link
Member

I see no problem with the PR, but I feel we need to warn that files may be overwritten.
Since all documents share the same name, all documents rendered to the same target directory are overwritten by the last one.

We could add a warning message in the code, or just state this in the documentation. wdyt?

Btw, AppVeyor passed OK.

@abelsromero
Copy link
Member

Also, it's worth mentioning that the path in outputFile is nicely appended to the source paths when using outputDirectory with and without preserveDirectories. 👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 60.177% when pulling 6b3c246 on tisoft:out-file into ed17a9e on asciidoctor:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 60.177% when pulling 6b3c246 on tisoft:out-file into ed17a9e on asciidoctor:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 60.177% when pulling 6b3c246 on tisoft:out-file into ed17a9e on asciidoctor:master.

@tisoft
Copy link
Contributor Author

tisoft commented Jul 21, 2017

I would add it to the documentation. It does the same as the -o, --out-file=OUT_FILE command line option from the asciidoc CLI command.

@abelsromero
Copy link
Member

Perfect! Do you want to it yourself to this PR? Or else, I can find some time tomorrow to do it.

@tisoft
Copy link
Contributor Author

tisoft commented Jul 22, 2017

I have added it to the README.adoc. I hope the wording is ok. 😏

I wasn't able to do the 汉语 translation, though. 😁

@coveralls
Copy link

coveralls commented Jul 22, 2017

Coverage Status

Coverage decreased (-0.03%) to 60.177% when pulling 0a55cca on tisoft:out-file into ed17a9e on asciidoctor:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 60.327% when pulling f5eda67 on tisoft:out-file into ed17a9e on asciidoctor:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jul 22, 2017

Coverage Status

Coverage increased (+0.1%) to 60.327% when pulling f5eda67 on tisoft:out-file into ed17a9e on asciidoctor:master.

README.adoc Outdated
@@ -130,6 +130,7 @@ When converting to HTML, images must be copied to the output location so that th
<resources>
----
outputDirectory:: defaults to `${project.build.directory}/generated-docs`
outputFile:: defaults to `null`, can be used to override the name of the generated output file. This is useful for backends, that create a single file, e.g. the pdf backend. All output will be redirected to the same file, the same way as the `-o, --out-file=OUT_FILE` option from the `asciidoc` CLI command.
Copy link
Member

Choose a reason for hiding this comment

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

You mean asciidoctor command right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, have fixed it. But I just checked, the asciidoc command supports the same parameter, so I wasn't wrong 😁

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. My comment is just cause I want to avoid controversy with the python implementation.

@abelsromero
Copy link
Member

I have one minor comment. I typo a guess :P
oh! and thanks a lot for adding a test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 60.327% when pulling 891bebb on tisoft:out-file into ed17a9e on asciidoctor:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jul 23, 2017

Coverage Status

Coverage increased (+0.1%) to 60.327% when pulling 891bebb on tisoft:out-file into ed17a9e on asciidoctor:master.

@abelsromero abelsromero merged commit 006bde2 into asciidoctor:master Jul 23, 2017
@tisoft tisoft deleted the out-file branch July 23, 2017 18:26
@mojavelinux
Copy link
Member

Since all documents share the same name, all documents rendered to the same target directory are overwritten by the last one.

I would say that it should only be used if there's exactly one input file. An alternative would be to use it, but warn if there's more than one input file. Either way, I agree it shouldn't silently work if there are multiple input files.

@abelsromero
Copy link
Member

I was happy with the mention in the README, but we can add a explicit warning in the build process.
To do it properly we need to take into consideration preserveDirectories as well, store the target paths and warn whenever a repeated one appears. That way the users know exactly what happened.
Also, on top of the warning, we could skip the rendering of the file, to 'fail' faster.

@mojavelinux
Copy link
Member

mojavelinux commented Jul 24, 2017

I was just thinking that if there is more than one file being processed (which the plugin should know), then it's a warning. This feature is essentially mutually exclusive to processing more than one file.

@abelsromero
Copy link
Member

But if files are in different directories and preserveDirectories is enabled, there's no collision. I admit it's odd, but still this causes no issue. Maybe I am overthinking?

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