Allow targets to specify output of the rule #1
Conversation
pandoc.bzl
Outdated
| "to_format": attr.string(), | ||
| "src": attr.label(allow_single_file = True, mandatory = True), | ||
| "to_format": attr.string(mandatory = True), | ||
| "output": attr.output(), |
There was a problem hiding this comment.
I was thinking. If we're making the output file configurable, maybe we could simply remove from_format and to_format entirely. Pandoc seems to automatically infer the intended source/destination format based on the file extension. If people need something custom, they could pass that in through options. What are your thoughts on this?
This would allow us to get rid of FORMAT_EXTENSIONS and PANDOC_FORMATS.
There was a problem hiding this comment.
I didn't know pandoc can infer the input & output formats.
I'm happy to remove the list of extensions, yes!
There is a little benefit in having the extensions in the bazel rule: we can fail early if a format is not specified.
The thing is: output is still not mandatory (my change is backward compatible).
Would you like to make output mandatory, then?
There was a problem hiding this comment.
Exactly! Making the output mandatory sounds good in that case.
There was a problem hiding this comment.
On second thought, removing --from_format would be a breaking change for the users of the rule. I'll make it optional and keep the existing behaviour. Feel free to remove really it in a subsequent change.
Also, if the rule knows the format, bazel can do an early check at analysis phase, rather than invoking pandoc which will then fail.
| to_format = fmt, | ||
| ) for fmt in PANDOC_FORMATS] | ||
|
|
||
|
|
There was a problem hiding this comment.
Could you please run buildifier on these files?
There was a problem hiding this comment.
Sure. Out of curiosiity, do you have a git presubmit hook for that? It seems buildifier alreadys returns 0.
There was a problem hiding this comment.
I don't have that set up right now. I usually just do a %!buildifier in Vim after making some edits. For a presubmit, we could always invoke buildifier and use cmp to compare changes against the input if we wanted one.
There was a problem hiding this comment.
There was a problem hiding this comment.
That's also one way to do it. Whatever works best for you :)
|
I've put too many things in this PR. Sorry! Let's split it in more actionable smaller pull requests. |
It's important that users of the rule can change the output. See #1 * Make "output": attr.output(mandatory = True), * Change the output of the sample generation to "readme_format.format"
regisd
changed the title
|
Please take another look. |
regisd
changed the title
|
PTAL |
from bazel-bin/docs/html.html to bazel-bin/docs/manual.html Thanks to ProdriveTechnologies/bazel-pandoc#1
* Use latest version of bazel_pandoc * Add a filegroup "docs" bazel build //docs Targets: bazel-bin/docs/manual.pdf bazel-bin/docs/html.html * Change output of //docs:html from bazel-bin/docs/html.html to bazel-bin/docs/manual.html Thanks to ProdriveTechnologies/bazel-pandoc#1
It's important that users of the rule can change the output.
For instance, we want to be able to generate "manual.html" and "manual.txt":
Today, the rule forces the output to be
html.htmlandtxt.txt.Of course, you can argue that the name of the rule is used, and I could have "manual.html". However, two labels cannot be identical, so there is no easy way to generate "manual.txt".
A workaround could be to add a genrule that copies "txt.txt" to "manual.txt".
Instead, I propose to replace the pattern generated
ctx.outputsby anoutputgiven inctx.attr.Also, this change makes
srcsingle file and mandatory.