Remove Kramdown overrrides #298
Conversation
jkempster34
force-pushed
the
fix-list-not-found-errors
branch
2 times, most recently
from
5a36eaf to
3e9f326
Compare
There was a problem hiding this comment.
Personally, I think this is cleaner than the previous approach.
Considering that we haven't managed to reproduce the errors (and that we possibly wouldn't even want to reproduce them in the test suite if it is a thread safety issue), are you confident that the existing tests cover this area as well as they could?
Testing a branch of the gem in Whitehall sounds workable. But if you're happy with this, we could just release it generally and monitor it?
| # The surrounding div is neccessary to control flow in `parse_block_html` and | ||
| # maintain the same functionality as a previous version of this extension. | ||
| # The surrounding div is neccessary to accurately identify legislative lists | ||
| # in post-processing. |
| # We override the parse_list method with a modified version where ordered lists are | ||
| # disabled (for use with legislative lists). The majority of the method body is copied | ||
| # over, only the LIST_START is changed. Previously, we dynamically modified some of the | ||
| # class-scoped variables used in this method; this provides a thread-safe alternative. |
There was a problem hiding this comment.
How would you feel about including a link to Github for the version of the method you copied in this comment? (e.g. maybe it was this one https://github.com/gettalong/kramdown/blob/REL_2_3_1/lib/kramdown/parser/kramdown/list.rb#L54 ?)
I've pulled the Govspeak out of the content items which are raising these errors, and all of them work as expected in Govspeak Preview. I think the test suite is fairly comprehensive when it comes to legislative lists. What would you recommend we call the release? |
Currently, in order to disable ordered lists for legislative lists, we modify the @@parsers class variable and the LIST_START constant of the Kramdown library. We are seeing a bunch of errors relating to these missing in production, such as: - NoMethodError: undefined method `start_re' for nil:NilClass (NoMethodError) if @src.check(@parsers[name].start_re) - Kramdown::Error: Unknown parser: list (Kramdown::Error) - NameError: uninitialized constant Kramdown::Parser::Kramdown::LIST_START Rather than modifying class variables and constants in a non thread-safe manner, this takes the simpler approch of copying the body of the `parse_list` method and making the neccessary changes required for disabling ordered lists. The downside of copying the majority body of the `parse_list` method may be offset by the fact that: - This is thread-safe - This is significantly simpler to understand - The list module of Kramdown is rarely updated. - The previous implementation also relied on the internals of Kramdown.
jkempster34
force-pushed
the
fix-list-not-found-errors
branch
from
3e9f326 to
ce1c192
Compare
We were revisiting Joe's old CTA and legislative list bug fixes in mobbing today. We wanted to know whether we could remove the code that we'd had to copy from the Kramdown codebase. The results turned out to look spookily similar to the code that Joe had originally removed (see: #298)
https://trello.com/c/lu30wpdq
Currently, in order to disable ordered lists for legislative lists, we modify the @@parsers class variable and the LIST_START constant of the Kramdown library.
We are seeing a bunch of errors relating to these missing in production, such as:
Rather than modifying class variables and constants in a non thread-safe manner, this takes the simpler approch of copying the body of the
parse_listmethod and making the neccessary changes required for disabling ordered lists.The downside of copying the majority body of the
parse_listmethod may be offset by the fact that:This repo is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.