feat: asciidoctor plugin #183
Conversation
A basic tasks for processing asciidoc files using AsciidoctorJ. This implementation is bare-bones, it does not cover Asciidoctor extensions and does not relate asciidoc attributes with Perun meta. The page title is not rendered by the Asciidoctor task, to mimic the behavior of the markdown extension.
|
For the backstory of this PR, please read #100 |
|
Any feedback? |
|
Hi Nico! I'm so glad you've found time to work on this. I apologize - you've caught me in the middle of a job transition (today is my second day) and I haven't had time to do a proper review. I should be able to do so this weekend, though. Sorry for the delay! |
|
@bhagany Thanks for letting me know. I'm excited to have something mergeable, so I'm eager to get it reviewed, that's all. But certainly it can wait some more, no problem. |
There was a problem hiding this comment.
code looks good to me. Waiting for @bhagany review before merging
| e extensions EXTENSIONS [str] "extensions of files to process" | ||
| m meta META edn "metadata to set on each entry"] | ||
| (let [{:keys [out-dir filterer extensions meta]} (merge +asciidoctor-defaults+ *opts*)] | ||
| (comp (yaml-metadata :filterer filterer :extensions extensions) |
There was a problem hiding this comment.
Since Asciidoc can use YAML front-matter itself, you might consider passing :keep-yaml true to yaml-metadata. That way, the YAML will still be present in the file when the asciidoctor* task sees it.
There was a problem hiding this comment.
I agree, but the issue arises that the default meta keys used in and !{Asciidoctor](http://asciidoctor.org/docs/user-manual/#attribute-catalog) are different. I was thinking of introducing a translation layer in a next version which contains the necessary translation layer, as I stated in the currently last comment on #100 For now I could indeed also just pass it through. What do you think, considering my comments? Both are fine to me.
src/io/perun/asciidoctor.clj
Outdated
| (:import [org.asciidoctor Asciidoctor Asciidoctor$Factory])) | ||
|
|
||
| (defn asciidoctor-to-html [file-content] | ||
| (.convert (Asciidoctor$Factory/create "") file-content {})) |
There was a problem hiding this comment.
This might not make much difference, but rather than creating the Asciidoctor interface on each call to asciidoctor-to-html, would it be more efficient to say, (def asciidcotor (Asciidoctor$Factory/create "")) as a top-level form, and then reuse the same object?
There was a problem hiding this comment.
Yes, but this fails the moment that environment-specific parameters are introduced like libraries. The issue of creating new containers came up on #100 and does hurt performance for large number of conversions.
I guess not creating new containers does limit the conversion process to a single core?
Probably the converter-specific settings have to be raised to the task level, so that a different container setting requires a different task to be created. Then inside the task the same converter (factory) can be used.
I will refactor this.
| @@ -400,6 +425,26 @@ This --- be ___markdown___.") | |||
| :msg "`pandoc` should parse HTML to markdown")) | |||
|
|
|||
| (add-txt-file :path "test.md" :content (nth input-strings 0)) | |||
| (p/asciidoctor :out-dir nil | |||
There was a problem hiding this comment.
I like that you're testing different parsing modes
There was a problem hiding this comment.
Then again I couldn't get the tests to fail based on content, so perhaps I'm using the framework incorrectly.
|
I'm very pleased with how clean this implementation is. I think it's a good base on which more Asciidoctor features can be added over time. Thanks! |
|
@bhagany Let me know what you think about the options for dealing with YAML, and if I'm missing out om some testing features in the testing framework. I'll refactor the initiation of new JRuby containers. |
| @@ -0,0 +1,13 @@ | |||
| (ns io.perun.asciidoctor | |||
src/io/perun/asciidoctor.clj
Outdated
| (.convert (Asciidoctor$Factory/create "") file-content {})) | ||
|
|
||
| (defn process-asciidoctor [{:keys [entry]}] | ||
| (perun/report-debug "asciidoctor" "processing asciidoctor" (:filename entry)) |
| :rm-originals true | ||
| :pod pod}))) | ||
|
|
||
| (deftask asciidoctor |
There was a problem hiding this comment.
By naming the task after the library you are using, you bind the implementation to the interface. A better task name would be ascidoc.
|
@MartyGentillon I see your point of view, but then again, there is also an outdated Python reference implementation of Asciidoc which is quite outdated Small parts of syntax in Asciidoctor are still improving, to end up at the end-goal of an 'unidoc' syntax. Also Asciidoctor has some Markdown support, and has the ability to load some plugins, like the terrific asciidoctor-diagram. So while I see where you are coming from, I'd rather leave it at the tool, for clarity. |
|
That seems like a fairly good argument to me. I wonder if it makes sense
to add aliases for those who don't care what Asciidoc interpreter they
use. It would give the best of both worlds, people woo don't care won't be
broken by a parser change, while those who do can use the advanced features.
…On Apr 21, 2017 11:00 AM, "Nico Rikken" ***@***.***> wrote:
@MartyGentillon <https://github.com/MartyGentillon> I see your point of
view, but then again, there is also an outdated Python reference
implementation of Asciidoc <http://www.methods.co.nz/asciidoc/> which is
quite outdated Small parts of syntax in Asciidoctor are still improving, to
end up at the end-goal of an 'unidoc' syntax. Also Asciidoctor has some
Markdown support, and has the ability to load some plugins, like the
terrific asciidoctor-diagram
<https://github.com/asciidoctor/asciidoctor-diagram>. So while I see
where you are coming from, I'd rather leave it at the tool, for clarity.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#183 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABQBZrBIz3oc2p31kaEsuwFP4fkJdKU6ks5ryO6-gaJpZM4M4Kpn>
.
|
Based on comments raised at hashobject#183 the asciidoctor container is now bound to the namespace and shared between processing calls.
To me the code is ready to merge in it's current state. Metadata would be the focus for the next iteration. |
|
Seems to already be decided, but because this task can parse different types of content, it seems to be closer to I do want to take a look at the tests, and I should have time tonight or tomorrow, but otherwise I agree this is ready to merge. I'm waiting for the legal team at my new job to sign off on my work on Perun, so I don't want to merge before that happens. If one of the other committers wants to after we figure out what's up with the tests though, I approve. |
|
Using aliases based on the type of content to be parsed is an interesting idea, and I think it deserves more thought. There would be quite a few aliases for |
|
@bhagany Thanks for the update. If somebody can verify I implemented the tests correctly, that would be great. |
|
@nicorikken I think the tests are implemented correctly. I was able to get them to fail by changing the |
|
I was expecting a summary of the test results, as I know from my Leiningen usage. But indeed I get error output if the testresults are incorrect: So in hindsight it was just my expectation, not the actual code. |
|
Okay, then I think this is ready to merge. Still haven't heard back from my legal department, but if another committer wants to merge before that happens, I'm good with that. |
|
Considering that all participants on this PR approved, can this be merged? |
|
I got permission from the legal department :) |
|
Nice! |
A basic tasks for processing asciidoc files using AsciidoctorJ. This implementation is bare-bones, it does not cover Asciidoctor extensions and does not relate asciidoc attributes with Perun meta.
The page title is not rendered by the Asciidoctor task, to mimic the behavior of the markdown extension.