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

Asciidoctor: Add support for inline callouts #566

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/ES/Util.pm
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ sub build_chunked {
# '-a' => 'attribute-missing=warn',
'-a' => 'asciidoc-dir=' . $asciidoc_dir,
$resources ? ( '-a' => 'resources=' . join(',', @$resources)) : (),
'-a' => 'inline-callouts',
'--destination-dir=' . $dest,
docinfo($index),
$index
Expand Down Expand Up @@ -211,6 +212,7 @@ sub build_single {
$private ? () : ( '-a' => "edit_url=$edit_url" ),
'-a' => 'asciidoc-dir=' . $asciidoc_dir,
$resources ? ( '-a' => 'resources=' . join(',', @$resources)) : (),
'-a' => 'inline-callouts',
# Disable warning on missing attributes because we have
# missing attributes!
# '-a' => 'attribute-missing=warn',
Expand Down
3 changes: 3 additions & 0 deletions resources/asciidoctor/lib/extensions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
require_relative 'elastic_compat_preprocessor/extension'
require_relative 'elastic_include_tagged/extension'

# This extensions is special because it is evil - just loading it is enough
require_relative 'inline_callout/extension'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then is it possible to only even load it when inline-callouts is defined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be possible but strange because we load these before we have a document to check what is defined. We'd have to load late which is indeed strange.


Extensions.register Added
Extensions.register do
# Enable storing the source locations so we can look at them. This is required
Expand Down
69 changes: 69 additions & 0 deletions resources/asciidoctor/lib/inline_callout/extension.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
require 'asciidoctor'

include Asciidoctor

##
# Enables inline callouts which asciidoc supports but asciidoctor doesn't.
# Filed as enhancement request at
# https://github.com/asciidoctor/asciidoctor/issues/3037
#
# Usage
#
# POST <1> /_search/scroll <2>
#
# NOTE: This isn't an asciidoctor extension so much as a hack. Just including
# the file causes us to monkey patch its code into asciidoctor. By default we
# don't change asciidoctor, but if you set the `inline-callouts` attribute so
# you need to *ask* for the change in behavior.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I am kind of reviewing with the -pedantic flag on here but... I still have some issues.

  1. The new sentence no longer really makes sense.

By default we don't change asciidoctor, but if you set the inline-callouts attribute so you need to ask for the change in behavior.

If it helps read it out loud. It's not conveying a coherent meaning.

  1. It kind of tells a lie.

By default we don't change asciidoctor

As also discussed below, logging behavior is being altered too, and that behavior change is not guarded by any check against (attr? 'inline-callouts') either, it always does it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logging behavior is only altered for the duration of the monkey patching which is right at startup. We restore the old verbose mode after completing the patch.

I'll see about rewriting the sentence.

#
module InlineCallout
InlineCalloutScanRx = /\\?<!?(|--)(\d+|\.)\1>/
InlineCalloutSourceRx = %r(((?://|#|--|;;) ?)?(\\)?&lt;!?(|--)(\d+|\.)\3&gt;)
InlineCalloutSourceRxt = "(\\\\)?&lt;()(\\d+|\\.)&gt;"
InlineCalloutSourceRxMap = ::Hash.new {|h, k| h[k] = /(#{::Regexp.escape k} ?)?#{InlineCalloutSourceRxt}/ }

# Disable VERBOSE so we don't log any warnings. It really isn't great to
# have to patch these in like this but it gets the job done and we're looking
# to get this into Asciidoctor proper. These methods are basically the same
# as the methods in asciidoctor but with new regexes.
old_verbose = $VERBOSE
$VERBOSE = false

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the actual scope of this change? If this extension is loaded, does the altering of logging only occur for inline callouts, or is loading this extension going to affect logging levels for the duration of the run? I'm afraid I know too little about ruby evaluation and loading rules to just know this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is global but I restore it after the fact. I think threads could be an issue here, but we're super single threaded at this point. I don't like it much though.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought for a while about asking if it could be guarded by a check for the inline-callouts argument, but then I think I have decided that basically we're always going to have that on, because we use them (or else this extension wouldn've have been needed to begin with) so it doesn't matter for us.

Which in turn means that we are always going to have log warnings suppressed, which is why I am so concerned about understanding the scope of how much time a complete run spends with the log levels tampered with vs. not.

Which brings me to ask whether patching instances is preferable, possibly in combination with moving the verbose tweak inside the overridden method, to ensure the scope is exactly what you're really after?

Parser.class_eval do
def self.catalog_callouts(text, document)
found = false
autonum = 0
callout_rx = (document.attr? 'inline-callouts') ? InlineCalloutScanRx : CalloutScanRx
text.scan(callout_rx) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the body here duplicated from what you are patching over? In particular if inline-callouts is not specified, I mean. Point being, in the absence of inline-callouts can this delegate to the normal implementation? (I'm probably showing how much ruby I don't really do, here...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can delegate, yeah. I wasn't partially because I figured this might be what it'd look like to make the change in asciidoctor itself. But it looks like they won't want this change until at least 2.0, which won't look like this anyway. So I can just delegate.

captured, num = $&, $2
document.callouts.register num == '.' ? (autonum += 1).to_s : num unless captured.start_with? '\\'
# we have to mark as found even if it's escaped so it can be unescaped
found = true
} if text.include? '<'
found
end
end

Substitutors.module_eval do
def sub_callouts(text)
autonum = 0
text.gsub(callout_rx) {
# honor the escape
if $2
# use sub since it might be behind a line comment
$&.sub(RS, '')
else
Inline.new(self, :callout, $4 == '.' ? (autonum += 1).to_s : $4, :id => @document.callouts.read_next_id, :attributes => { 'guard' => $1 }).convert

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too long and too complicated, it has a ?: inside the argument list, it uses magic gsub numbers, it has a hash assignment inside of a hash assignment...

Formatting may be enough to address most of this, a variable or two might also be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I comes from upstream so I dind't touch it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't have written it this way though.

end
}
end

def callout_rx

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I freely admit this is picky, but this is both a) defined after it is used, and b) defined after a previous block of code also defined a different variable with the exact same name. I know it made me have to squint at it twice as a result. Presuming the order doesn't matter, and I'd be horrified if it did, I think it would read more clearly if it were swapped in order with sub_callouts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is actually to a different place. This one is in Substitutors module and the other is in the Parser class. I have them in this order because it is the order in which they are run so it made sense to me. Would it be less confusing if the method were a variable? I can do that, it just didn't look as nice to me.

if attr? 'line-comment'
((attr? 'inline-callouts') ? InlineCalloutSourceRxMap : CalloutSourceRxMap)[attr 'line-comment']
else
(attr? 'inline-callouts') ? InlineCalloutSourceRx : CalloutSourceRx
end
end
end
$VERBOSE = old_verbose
end
57 changes: 57 additions & 0 deletions resources/asciidoctor/spec/inline_callout_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
require 'inline_callout/extension'

RSpec.describe InlineCallout do
it "enables support for inline callouts if requested" do
attributes = {
'inline-callouts' => '',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What meaning does '' have as a value? In many languages that value is false-y.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In asciidoctor flags attributes are usually controlled by being defined at all. It is weird, but it feels more like the way the rest of asciidoctor works.

}
input = <<~ASCIIDOC
----
POST <1> /_search/scroll <2>
----
<1> words
<2> other words
ASCIIDOC
expected = <<~DOCBOOK
<preface>
<title></title>
<screen>POST <co id="CO1-1"/> /_search/scroll <co id="CO1-2"/></screen>
<calloutlist>
<callout arearefs="CO1-1">
<para>words</para>
</callout>
<callout arearefs="CO1-2">
<para>other words</para>
</callout>
</calloutlist>
</preface>
DOCBOOK
expect(convert(input, attributes)).to eq(expected.strip)
end

it "does not enable support for inline callouts by default" do
input = <<~ASCIIDOC
----
POST <1> /_search/scroll <2>
----
<1> words
<2> other words
ASCIIDOC
expected = <<~DOCBOOK
<preface>
<title></title>
<screen>POST &lt;1&gt; /_search/scroll <co id="CO1-1"/></screen>
<calloutlist>
<callout arearefs="">
<para>words</para>
</callout>
<callout arearefs="CO1-1">
<para>other words</para>
</callout>
</calloutlist>
</preface>
DOCBOOK
actual = convert input, {}, eq('WARN: <stdin>: line 4: no callout found for <1>')
expect(actual).to eq(expected.strip)
end
end