Skip to content

Conversation

@ashmaroli
Copy link
Contributor

This is helpful for downstream libraries such as Jekyll where a single configuration is used to process numerous kramdown documents.

To illustrate, consider the following flow:

# given: `documents` is an array of 1000 Markdown pages

kd_opts = {:smart_quotes => "lsquo,rsquo,ldquo,rdquo"}
outbox  = documents.map { |doc| Kramdown::Document.new(doc.content. kd_opts).to_html }

By running the above script, there's going to be a 1000 instances of Kramdown::Document (obviously).
But each of those instance is going to use the same set of options.

However, each of those instance is going to allocate a new %w[lsquo rsquo ldquo rdquo] via val.split(/,/) during the validation. The arrays so obtained are valid but in spite of that, each constituent element is consequently forced to initialize an ArgumentError via Kernel#Integer and then fallback to returning original element.


  • The proposed solution is therefore to define a constant and tally given option value against it. If the two objects are equal in both the size and order of elements, return the given value otherwise, proceed to existing logic.
  • I added another constant to similarly optimize situations where the option was a valid array in the first place (instead of the default csv).

Attn: @gettalong

P.S. Additionally, the else block could be optimized to not rescue an intermediate exception, but I chose to defer that for future.

@ashmaroli ashmaroli force-pushed the no-process-valid-smart-quotes branch from 99843cf to 3a9a78b Compare May 15, 2019 06:40
@gettalong gettalong self-assigned this May 15, 2019
@gettalong
Copy link
Owner

Thanks for the changes - I have cherry-picked your commit!

@gettalong gettalong closed this May 15, 2019
@ashmaroli ashmaroli deleted the no-process-valid-smart-quotes branch May 15, 2019 10:50
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.

2 participants