Enhance #80
Enhance #80
Conversation
There is no need for different candidates for Array and Pod::Block if the implementation is the same. Create a single candidate for the two types where `load` call is necessary and use nextwith to pass the execution there. This makes code shorter and easier to think about, while not losing a lot in readabilty.
Titlecase suggests type, while in fact we have a routine.
Also, use a more modernand hopefullly robust code style, for now for 012-multi.t only.
Over the years, code style became a bit inconsistent with different styling of spacing, indentation and such tiny details. This commit makes the style more unified, thus encouraging positive changes and making it easier for the reader to digest.
For now, this one looks like a cosmetic change and OOP cargo cvlting, as things Just Work in a procedural style and there is no need for the pesky classes. However, having data hidden allows us to reason clearer about what code works with what. While it might sound ephemerical enough not to bother, this also allows a much easier renderer extending for our clients.
No big need for all the multis, golf Pod::Raw calculation as well.
Prior this, the code gathered all the headers and rendered them into text in place. While this is not really wrong, it is not future-proof, because actual content (TOC structure) is tied to its presentation (particular HTML) and if the user wants to add a mere class or a wrapper div, everything, including heading gathering logic, must be re-implemented, because do-toc sub is not extendable. So, here we introduce TOC::Calculator which has `calculate`, `render` and `render-heading` methods. First one calculates a structure, representing the TOC of the $pod in question and two other solely do rendering, thus making it easier to extend and digest. Also, increase test coverage a bit.
|
Thanks! |
|
Please review. I also observe a surprising (to me) noticeable slowdown, e.g. 012-multi run for 10 times here takes around 2.7-2.8 seconds, but version on master takes about 2.1 (this can be explained by the fact that amount of code run by the test differs, but still even if you run Seems like using |
|
Isn't the |
|
@lizmat it is, overall some tests are suboptimal here, but it is used only on testing, so does not make the actual code perf worse, I think. |
|
Since Mustache was incorporated, everything slowed down by an order of
magnitude. I think @antoniogamiz is working on al alternative, but for the
time being I'm sticking to the old version.
El vie., 18 sept. 2020 a las 11:20, Altai-man (<notifications@github.com>)
escribió:
… @lizmat <https://github.com/lizmat> it is, overall some tests are
suboptimal here, but it is used only on testing, so does not make the
actual code perf worse, I think.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#80 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAD5G3UC6KBT5XMI4GZUTSGMQ4VANCNFSM4RRUFXCA>
.
--
JJ
|
|
Mustache was incorporated a long time ago, but it was not really used. The problem was the parsing phase of mustache, which was solved by @softmoth. The current version of Documentable takes (more or less) the same time than the older. |
- Make node renderer overloadable - Make TOC renderer overloadable - Make do-footnotes method public, thus overloadable - Export a couple of universally used subs Tiny tweaks: - Make names in assemble-list-items more readable - Tweak heading handler logic a bit - Remove `Pod::To::HTML` module wrapper, let's see if it breaks anything
While we are striving to preserve backward compatibility everywhere, a version bump fulfills its purposes here.
|
Still in draft state? Or is it ready for review? |
|
For now I have all the features I currently need, so can be reviewed, yes. |
|
For reviewers: I suggest going commit by commit, instead of a full diff, which became cumbersome due to file renaming. I tried to make commit messages nice enough as well. |
|
Anyone? |
| @@ -1,10 +1,17 @@ | |||
| use Test; # -*- mode: perl6 -*- | |||
| use Test; | |||
There was a problem hiding this comment.
I really need the mode lines, but, hey, not a big deal.
There was a problem hiding this comment.
Maybe we should switch to .rakutest extensions then?
|
Did you do any kind of benchmark? |
|
Sorry for the delay.
|
Run test suite and it got somewhat slower, but I also increased number of tests somewhat, so hard to judge. What I know is that I generate pages for the website using this branch and it is pretty okay for me. Not sure how it would react on building whole documentation at once, but we can hopefully resolve it together should an issue arise. |
|
FWIW, I did a profile on converting one of my more extensively documented modules: most of the time is spent in Barring some magical insights, |
|
Thanks for the insight, @lizmat !
I opened #82 because in my tests Cro::WebApp templates are faster than Template::Mustache (not by a lot, about 2x improvement), but during original design mustache templates were quite tightly baked in, so ability to use custom engines should be added very carefully not to break existing tools. I am also very pleased to note that Pod manipulation and Documentable::Registry is fast enough (though can be made faster, I suspect, as the code apperars to be as suboptimal in some places). |
|
This PR might also need to be reverted. We really need more extensive tests for this module, including the fact that render needs to be called as a subroutine or a class method, besides an instance method. |
No need if existing issues will be fixed. |
First part of changes. Second will be focused on adding some necessary features, while this one is focused on preparations and making the code more factored.