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

feat(asciidoctor): basic asciidoc plugin #100

Closed
wants to merge 10 commits into from

Conversation

nicorikken
Copy link
Contributor

@nicorikken nicorikken commented Sep 13, 2016

The asciidoc plugin uses AsciidoctorJ from the Asciidoctor project
to bring Asciidoc rendering to Perun.
This ticket is related to issue 49 on GitHub:
#49

Left to do:

  • Enable AsciidoctorJ libraries (like asciidoctor-diagram). In particular, how to handle the resulting image files? Help desired! Images are now generated on root, will have to be handled properly
  • Forward already available metadata to the document. Metadata from the options is now forwarded
  • Read metadata from the document, apart from the YAML
  • Link AsciidoctorJ containers to a pod for improved performance. Help desired! Eventually linked to the fileset
  • Get image generation by asciidoctor-diagram handled properly, to make sure they end up in the end result, and at the right place. Help desired!
  • Change the metadata keywords for compatibility with the rest of Perun
  • Have the defaults generate HTML similar to the Markdown HTML (which headers and H1's included).
  • Expand readme with info on the Asciidoctor support and how to use it.

The asciidoc plugin uses AsciidoctorJ from the Asciidoctor project
to bring Asciidoc rendering to Perun.

ROOM FOR IMPROVEMENT
AsciidoctorJ has plenty of extension options, like including other
Asciidoctor libraries (like 'asciidoctor-diagram') and including
gems. Also attributes can be set, determining how the HTML is
rendered. These Asciidoctor features make it flexible and powerful
but these features need to be enabled still.
Another potential improvement is the linking of Asciidoc containers
(JRuby containers) to pods to prevent initialization. This will
require further changes in the perun.clj file related to the pod
dispatch.

This ticket is related to issue 49 on GitHub:
hashobject#49
@nicorikken
Copy link
Contributor Author

nicorikken commented Sep 15, 2016

It is getting there. Basic tests are included.

@martinklepsch
Copy link
Contributor

Link AsciidoctorJ containers to a pod for improved performance. Help desired!

This is relatively easy, right now you have:

(pod/with-call-in @pod (io.perun.contrib.asciidoctor/parse-asciidoc ~ad-files ~(merge +asciidoctor-defaults+ options)))

what you can do is make that parse-asciidoc function take a container as argument and define the container inside the pod like so:

(pod/with-call-in @pod (def container (new-container ~options)))

Then you can pass that defined container to the parse-asciidoc function.
There are also a few differences betwen with-call-in and with-eval-in (and probably some others so you might need to test which one you need).

Also here's a snippet where something similar is done.

@nicorikken
Copy link
Contributor Author

Image rendering works, although they now get dropped in the root folder.

@nicorikken
Copy link
Contributor Author

@martinklepsch I started working on linking the asciidoctor-container to the pod, but I guess I'm having issues with the namespaces. I'd like to leave the new-adoc-container function in the io.perun.contrib.asciidoctor namespace, to avoid bringing in other dependencies on AsciidoctorJ and the normalize-options helper. I run into either Unable to resolve symbol: container in this context or can't resolve symbol (def) errors. Also I'm not sure whether or not I should unquote the newly created container. I'll probably have to read up on the pods to get this straight.

@nicorikken
Copy link
Contributor Author

@martinklepsch I committed my current work in progress, so you can have a look: nicorikken@6b53756

@nicorikken
Copy link
Contributor Author

Metadata parsing is almost complete, only need to fix the existing tests. I'll commit it later today.

@nicorikken
Copy link
Contributor Author

Will there be multiple pods handling the Asciidoctor fileset? Or will the files be parsed concurrently? Otherwise the asciidoctor container could also be defined in the parse-asciidoc function which takes the fileset and parses multiple files. At least it would make the container creation outside the pod structure easier to understand, if only for me.

@nicorikken
Copy link
Contributor Author

I did a test to find out if some level of concurrency makes sense for conversion. Running the clojure.core.reducers/map instead of map makes little difference.
screen shot 2016-09-16 at 10 19 55
And of course I tried creating a JRuby container for each document to render. Other then being very busy, I killed it because it was still busy 15 minutes in.
screen shot 2016-09-16 at 10 01 57

@nicorikken
Copy link
Contributor Author

All the previous checkmarks are checked! 🎉 I can still improve on the metadata, and ensure that the title is included. Also I'd like to align the keywords with the SPEC.md file for compatibility with other parts of Perun.

@nicorikken
Copy link
Contributor Author

I've now got the metadata working properly (I had to strip the frontmatter before handing it over to the .readDocumentStructure for parsing the header information. The same goes for the .convert itself, as the skip-front-matter attribute fails to influence the document conversion somehow. Maybe that only affects a full-document parsing (.convertFile rather than .convert). On the same topic I'll have to look into the header and footer parsing. Now the H1 and author attributes are skipped fully, in contrast with the Markdown parser. There is an option for including the title (H1), but that would still leave out the author and revision information. Maybe that would would still be the most sane default.

@nicorikken
Copy link
Contributor Author

Very close now, only image handling left.

@nicorikken
Copy link
Contributor Author

nicorikken commented Sep 20, 2016

screen shot 2016-09-20 at 21 46 30

The image generation using the `asciidoctor-diagram` library can use the `imagesoutdir` attribute to determine where the final images will end up. Otherwise they will end up at the directory from which the task is called. The `imagesdir` should match the `imagesoutdir` to some extent, to ensure the final HTML will refer to the correct files. _But how to get this working in Boot...?_ Multiple solutions exist to get to the image output directory https://github.com/asciidoctor/asciidoctor-diagram#image-output-location

@nicorikken
Copy link
Contributor Author

I'm also working on an improved version of the YAML frontmatter strip action, to properly handle files without frontmatter. When those test run, I'll commit it, and only the image handling seems left.

@nicorikken
Copy link
Contributor Author

Got the tests working (apparently map destructuring in the let block was giving issues), so now only documenting and image inclusion remains.

@nicorikken
Copy link
Contributor Author

Regarding the image generation, I should be able to set the imagesoutdir to a new tmp-dir! and I should be able to commit them on the imagesdir and outdir stated in the main Asciidoctor file.

@nicorikken
Copy link
Contributor Author

I was trying to get the path from the Asciidoctor file being processed by the Boot Fileset methods (tmp-path f) or (tmp-file f) but both result in stack-traces:

  • clojure.lang.ExceptionInfo: No implementation of method: :path of protocol: #'boot.tmpdir/ITmpFile found for class: clojure.lang.PersistentArrayMap
  • clojure.lang.ExceptionInfo: No implementation of method: :file of protocol: #'boot.tmpdir/ITmpFile found for class: clojure.lang.PersistentArrayMap
    Can somebody help me out to get the image handling compatible with Boot? Another option is to remove the asciidoctor-diagram library from the defaults and merge this PR with the mention that images handling is not yet worked out.

@nicorikken
Copy link
Contributor Author

nicorikken commented Oct 5, 2016

I'm getting there. Just fixed a but in merging maps (forgot to provide a function to merge-with 😛 )

TODO

  • Convert options in lowest functions (normalize-options)
  • Get it working for build-dev, not only for tests
  • Make a tmpdir for the images and provide to the parse-adciidoc
  • Add image directories in the in- and output for tmpdir

@nicorikken
Copy link
Contributor Author

Now that I have fixed controlling the output directory, I could also set it to the :base_dir such that they are next to the parsed .adoc files. More in the way that the proposed PR #79 handles images. Maintainers, any preferences?

@nicorikken
Copy link
Contributor Author

nicorikken commented Oct 18, 2016

I did spend some more evenings to clean up and get images working. I found out that my render is somehow broken. Pages are rendered, committed to the fileset, but not synced to the target dir. This concerns the HTML renders, also for Markdown. Unless I get it working again, debugging Asciidictor fileset handling is impossible.

@Deraen
Copy link
Contributor

Deraen commented Oct 19, 2016

Did you include target task in the pipeline? Boot 2.6 no longer automatically writes target dir.

@nicorikken
Copy link
Contributor Author

No I did not. Thanks for mentioning, I'll give it another go very soon, hopefully this PR is now actually in a working state.

@nicorikken
Copy link
Contributor Author

Alright, back on track. Somehow the image is currently ending up in the src directory, and the perhaps the metadata is not handled properly. Expect me back with fixes soon.

@bhagany bhagany mentioned this pull request Nov 26, 2016
@nicorikken
Copy link
Contributor Author

@bhagany thanks for taking my implementation issues and improving the general ecosystem!

@nicorikken
Copy link
Contributor Author

Just a heads up, my progress stalled due to a new laptop and a temporary project. I'd like to get something up and running before Fosdem, so perhaps I'll split the PR to ignore the asciidoctor diagram extension for now. Or I'll generate them, but dump them in a fixed directory for now.

@bhagany
Copy link
Contributor

bhagany commented Jan 15, 2017

@nicorikken glad to hear you're still in-progress on this, I think this feature is a great addition to Perun. I don't really know much about asciidoctor, but I like your idea of splitting the PR.

I don't know if you saw, but #132 changes the way content-parsing tasks are written. It should work for asciidoctor, but I haven't actually tried to make it work. Feel free to ping me here or on the Clojurians Slack if that gets merged and you run into issues.

Thanks for continuing to work on this!

@nicorikken
Copy link
Contributor Author

Picked this project up again. As it has gotten terribly behind on master, and I gained much more Clojure experience since my last few commits, I'll restart from master. I'm glad the Pandoc converter has made its way into the project, as I can expect the abstractions to be more stable and sensible. E.g. I'll be using the generic YAML parser. Also I like the hardcoded set of markdown extension as a pattern. I'll probably do the same, rather then increasing complexity by making it more dynamic. Also I'll ignore the asciidoctor-diagram for now then, to further lower complexity.

@bhagany
Copy link
Contributor

bhagany commented Apr 3, 2017

Sounds like a solid approach to me. Thanks again for working on this, and I look forward to getting this merged!

@nicorikken
Copy link
Contributor Author

I just finished setting out the basic task structure: 62f409a...nicorikken:asciidoctor-plugin-redo
The structure is totally different now, but the abstraction from Boot filesets makes me more comfortable to add another tasks. I'd probably still have to dive in when dealing with the asciidoctor-diagram plugin in the future, but as said before, I'll leave that for now.
Next step will be to reintroduce the Asciidoctor code I wrote earlier to do the actual conversion.

@nicorikken
Copy link
Contributor Author

I just finished the basic implementation: nicorikken@99283e5
It lacks some unit-tests, otherwise it is good to merge.

@nicorikken
Copy link
Contributor Author

Just added some basic tests. They do however not fail when loading wrong content to test against, so perhaps I'm testing it wrong. Either way, the new PR can be found here: #183

@nicorikken
Copy link
Contributor Author

After this basic task is merged, I'd like to extend it by syncing the Asciidoctor attributes and Perun meta. This would be something like:

;; For asciidoctor, YAML meta and Asciidoctor attributes to resulting meta
{:meta (->> meta
            (merge yaml-metadata)
            meta->attr
            (fn [attr] (.getAttributes (.getHeader (.readDocumenStructure container file-content {:attributes attr}))))
            attr->meta)}

;; For asciidoctor*, YAML meta and Asciidoctor attributes used in converting, not added to resulting meta
(->> meta
     (merge yaml-metadata)
     meta->attr
     (process-asciidoctor file-content {:attributes attr}))     

@nicorikken
Copy link
Contributor Author

Reading about the YAML frontmatter handling in the Asciidoctor user manual, the section on the handling by Awestruct shows an increased complexity, as the precedence of attributes can be changed by adding an @ after the assignment: http://asciidoctor.org/docs/user-manual/#configuring-attributes-for-awestruct I believe the solution for Perun should be more straightforward than that. If necessary control can be provided what keys are passed on in the metadata.

@bhagany
Copy link
Contributor

bhagany commented Apr 27, 2017

@nicorikken Is this PR still relevant, after merging the other one?

@nicorikken
Copy link
Contributor Author

No, although I'll probably come back to these comments for implementing metadata and asciidoctor-diagram. But I can do so while this is closed. Let's keep it clean.

@nicorikken nicorikken closed this Apr 27, 2017
@nicorikken
Copy link
Contributor Author

I spent the last few days rebuilding my blog, and as a result I now have a working solution for metadata extraction, and am working on getting a limited image-generation in place. Expect an asciidoctor PR soon.

@podviaznikov
Copy link
Member

podviaznikov commented Aug 18, 2017 via email

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.

5 participants