Asciidoctor: Support for "open in widget" #627

nik9000 · 2019-02-21T16:36:35Z

This'll take a while to describe properly. Let's start with the goal
first, talk about the AsciiDoc implementation, then move on to the
Asciidoctor implementation, then talk about why we need a compatibility
layer, then describe the compatibity layer, and finally round out this
book of a commit message by describing some of the more esoteric usages
of this that this change currently supports and our plan for dropping
this support in the future.

== The goal

Certain snippets in Elastic's docs are special and we'd like to decorate
them with buttons. These buttons allow opening the snippets in developer
tools or transforming them into cURL commands. I'm calling all of the
stuff that makes these buttons an "open in widget".

== AsciiDoc implementation

Because of AsciiDoc's fairly limited ability to customize it we
triggered these snippets by adding special comments after the code
blocks that we'd like to decorate that look like this:

[source,js]
----
GET /
----
// CONSOLE   <----- This is the comment

We customize AsciiDoc to emit all comments as <remark> elements in the
generated xml and then use custom xslt to recognize remarks that look
like <remark>CONSOLE</remark> to trigger the widget.

These buttons need the contents of the snippet, GET / in the example
above, to be accessible in a file. We implement this by post-processing
the generated html in the perl code to extra these snippets to files,
rewriting the html that generates the widget to point to the file.

== Asciidoctor implementation

Asciidoctor feels strongly that comments shouldn't have semantic
meaning. I'm on board with that. So, to trigger the widget in
Asciidoctor you use:

[source,console]
----
GET /
----

Note that the language, which was js in AsciiDoc is now console.
This language is what triggers the widget. This feels good to me because
all snippets that are in the "console" language really do want this
decoration. And because the snippets really aren't javascript. They
just often have json in them.

We recognize these :listing blocks using a treepreprocessor and built
the snippet file when processing the Asciidoctor AST. We also rewrite
the docbook that is generated by these blocks to contain a link to the
extract snippet. Finally, we also use custom xslt to extract that link
and render the widget. The xslt is less hairy than the one used to find
the <remark>s.

== Why we need compatibility

It'd be fairly simple to change // CONSOLE to [source,console] on a
page by page basis, but we have thousands of uses of // CONSOLE across
dozens of books and dozens of branches. In addition, AsciiDoc doesn't
understand [source,console] which'd make changing these a thing that
you'd have to do at the same time as you switched the book from AsciiDoc
to Asciidoctor. Beyond that, Elasticsearch automatically extracts
snippets with the // CONSOLE comment and turns them into tests.

There are just too many moving parts for a hard cut over from
// CONSOLE to [source,console]. So I built a compatiblity layer in
Asciidoctor

== How the compatibilty layer works

We use two customizations in Asciidoctor to make the compatibility layer
go, one that recognizes comments shaped like // CONSOLE and turns them
into a literal // CONSOLE using the pass macro like so:
pass:[// CONSOLE]. The next one picks up source blocks that are
immediately followed by pass:[// CONSOLE] and switches the language to
console.

Something like this begs for away to tell the user "you have 1232
warnings that you have to fix". Like linting but for your docs. I'll be
thinking more about this in the coming weeks. But for now there is not
warning when you use the compatibility layer.

== Esoteric forms of the "open in widget"

Kibana's docs have snippets like:

[source,js]
--------------------------------------------------
GET api/logstash/pipeline/hello-world
--------------------------------------------------
// KIBANA

Note the // KIBANA. This is like // CONSOLE but it is for snippets
that should be sent to Kibana's API instead of Elasticsearch's. It is
debateable if kibana is really a different language than console,
but it is at least a different dialect. Either way, Asciidoctor triggers
the "open in widget" for these snippets with [source,kibana].

Older versions of the documentation have snippets like this:

[source,js]
--------------------------------------------------
GET /
--------------------------------------------------
// AUTOSENSE

These open the command in an older tool named "sense" instead of the
developer console. The developer console "grew out of" sense but hasn't
been called that for a long time. In any case, it exists for
compatibility with old books.

The Definitive Guide has snippets that look like:

[source,js]
--------------------------------------------------
GET /
--------------------------------------------------
// SENSE:a/path/to/some.json

These are snippets that render as GET / but when you open them in
"sense" they have the contents of a/path/to/some.json which is
supposed to be similar. This isn't widely used and I personally think it
is super confusing, at least the way that we've implemented it now. So I
log a warning whenever you try to do this which should prevent folks
from doing it accidentally.

== Follow ups

This work begs for at least two follow up changes:

A warnings management system so old books can continue to use the
backwards compatibility features but new books will be forced to use
native Asciidoctor features.
More in depth integration testing. Right now we build
README.asciidoc with AsciiDoc and Asciidoctor which is great, but it is
difficult to assert that the results are "close enough" with the tools
that we have. We can do better and this change makes it obvious that we
must do better.

This'll take a while to describe properly. Let's start with the goal first, talk about the AsciiDoc implementation, then move on to the Asciidoctor implementation, then talk about why we need a compatibility layer, then describe the compatibity layer, and finally round out this book of a commit message by describing some of the more esoteric usages of this that this change currently supports and our plan for dropping this support in the future. == The goal Certain snippets in Elastic's docs are special and we'd like to decorate them with buttons. These buttons allow opening the snippets in developer tools or transforming them into `cURL` commands. I'm calling all of the stuff that makes these buttons an "open in widget". == AsciiDoc implementation Because of AsciiDoc's fairly limited ability to customize it we triggered these snippets by adding special comments after the code blocks that we'd like to decorate that look like this: ``` [source,js] ---- GET / ---- // CONSOLE <----- This is the comment ``` We customize AsciiDoc to emit all comments as `<remark>` elements in the generated xml and then use custom xslt to recognize remarks that look like `<remark>CONSOLE</remark>` to trigger the widget. These buttons need the contents of the snippet, `GET /` in the example above, to be accessible in a file. We implement this by post-processing the generated html in the perl code to extra these snippets to files, rewriting the html that generates the widget to point to the file. == Asciidoctor implementation Asciidoctor feels strongly that comments shouldn't have semantic meaning. I'm on board with that. So, to trigger the widget in Asciidoctor you use: ``` [source,console] ---- GET / ---- ``` Note that the language, which was `js` in AsciiDoc is now `console`. This language is what triggers the widget. This feels good to me because all snippets that are in the "console" language really do want this decoration. And because the snippets really *aren't* javascript. They just often have json in them. We recognize these `:listing` blocks using a treepreprocessor and built the snippet file when processing the Asciidoctor AST. We also rewrite the docbook that is generated by these blocks to contain a link to the extract snippet. Finally, we also use custom xslt to extract that link and render the widget. The xslt is less hairy than the one used to find the `<remark>`s. == Why we need compatibility It'd be fairly simple to change `// CONSOLE` to `[source,console]` on a page by page basis, but we have thousands of uses of `// CONSOLE` across dozens of books and dozens of branches. In addition, AsciiDoc doesn't understand `[source,console]` which'd make changing these a thing that you'd have to do at the same time as you switched the book from AsciiDoc to Asciidoctor. Beyond *that*, Elasticsearch automatically extracts snippets with the `// CONSOLE` comment and turns them into tests. There are just too many moving parts for a hard cut over from `// CONSOLE` to `[source,console]`. So I built a compatiblity layer in Asciidoctor == How the compatibilty layer works We use two customizations in Asciidoctor to make the compatibility layer go, one that recognizes comments shaped like `// CONSOLE` and turns them into a literal `// CONSOLE` using the `pass` macro like so: `pass:[// CONSOLE]`. The next one picks up source blocks that are immediately followed by `pass:[// CONSOLE]` and switches the language to `console`. Something like this *begs* for away to tell the user "you have 1232 warnings that you have to fix". Like linting but for your docs. I'll be thinking more about this in the coming weeks. But for now there is not warning when you use the compatibility layer. == Esoteric forms of the "open in widget" Kibana's docs have snippets like: ``` [source,js] -------------------------------------------------- GET api/logstash/pipeline/hello-world -------------------------------------------------- // KIBANA ``` Note the `// KIBANA`. This is *like* `// CONSOLE` but it is for snippets that should be sent to Kibana's API instead of Elasticsearch's. It is debateable if `kibana` is really a different language than `console`, but it is at least a different dialect. Either way, Asciidoctor triggers the "open in widget" for these snippets with `[source,kibana]`. Older versions of the documentation have snippets like this: ``` [source,js] -------------------------------------------------- GET / -------------------------------------------------- // AUTOSENSE ``` These open the command in an older tool named "sense" instead of the developer console. The developer console "grew out of" sense but hasn't been called that for a long time. In any case, it exists for compatibility with old books. The Definitive Guide has snippets that look like: ``` [source,js] -------------------------------------------------- GET / -------------------------------------------------- // SENSE:a/path/to/some.json ``` These are snippets that render as `GET /` but when you open them in "sense" they have the contents of `a/path/to/some.json` which is supposed to be similar. This isn't widely used and I personally think it is super confusing, at least the way that we've implemented it now. So I log a warning whenever you try to do this which should prevent folks from doing it accidentally. == Follow ups This work begs for at least two follow up changes: 1. A warnings management system so old books can continue to use the backwards compatibility features but new books will be forced to use native Asciidoctor features. 2. More in depth integration testing. Right now we build README.asciidoc with AsciiDoc and Asciidoctor which is great, but it is difficult to assert that the results are "close enough" with the tools that we have. We can do better and this change makes it obvious that we must do better.

nik9000 · 2019-02-21T17:48:45Z

README.asciidoc

@@ -1051,38 +1083,6 @@ The local web browser can be stopped with `Ctrl-C`.

 ================================

-==== Custom Console snippets


I've dropped the docs for this because I think, at least as we have it now, it is super confusing. It isn't compatible with Elasticsearch's tests or COPY AS cURL and I expect it is pretty surprising in general.

The reason these custom blocks exist is for the Definitive Guide, where we wanted to display a small amount of JSON but then click through to console to show the full example, including setup etc. Other than the Def Guide, it is not really used anywhere else

Yeah! I tracked that down.

We've talked on and off about having some way of hiding something like these custom blocks but in a way with visual feedback for what you are getting. I'm not sure the right way to do it but I could image these making a comeback eventually.

But as I have it now I'm just dropping the docs for them because I don't want people using them without really knowing what they are doing.

clintongormley · 2019-02-21T18:46:50Z

README.asciidoc

+the Elaticsearch repository because it can recognize it to make tests. The
+"Asciidoctor" way is preferred in other books, but only if they are built with
+"Asciidoctor". Try it first and if it works then use it. Otherwise, use the
+"AsciiDoct" way.


Suggested change

"AsciiDoct" way.

"AsciiDoc" way.

nik9000 · 2019-02-25T22:20:06Z

@ddillinger, could you add this to your review queue? I think the idea is sound even if it does create two ways to do it. I think this is one of those things like the inline callouts where we will have to change. But unlike the inline callouts we'd change after switching to asciidoctor because the compatibility layer isn't too funky and because we have zillions of these.

ddillinger · 2019-02-26T14:46:36Z

resources/asciidoctor/lib/open_in_widget/extension.rb

+      if File.readable? normalized
+        copy_snippet block, normalized, snippet_path
+      else
+        logger.warn message_with_context "can't read snippet from #{normalized}", :source_location => block.source_location


Is warn sufficient here, or should it fail more aggressively? This looks like a pretty bad state to be in and seems like it would produce broken docs (pieces missing).

warn is the level that Asciidoctor fails when it can't include a referenced file. We have to fail the asciidoctor run if it emits any warnings. Which, now that you mention them, would be another great integration test. Which I'll do in a separate PR because it'll have a bigger splash radius.

Hmm - I've rechecked this. Asciidoctor actually spits out an ERROR for missing files. I can switch to that. But it still keeps going which isn't really a terrible thing because it allows you to collect all of the failures. Sometimes that results in kind of a mess, but often it is quite helpful.

Yeah for sure that is a better behavior, we just also really want error when something goes as sideways as "an entire whole thing was just not there" :)

asciidoc is the one that warns! asciidoctor makes an error..... I had my programs crossed.

ddillinger · 2019-02-26T14:54:48Z

resources/asciidoctor/spec/elastic_compat_tree_processor_spec.rb

+  [
+    %w[CONSOLE console],
+    %w[AUTOSENSE sense],
+    %w[KIBANA kibana],


Unlike the other test, this one lacks a SENSE case; is that okay?

Hmmmmm...... When I wrote this I distinctly remember thinking that it was ok. On further reflection it is not OK. We use SENSE here.

resources/website_common.xsl

nik9000

Thanks for reviewing @ddillinger! I'll fix it up and then get to work on the integration test for warnings.

nik9000 · 2019-02-26T15:05:21Z

resources/asciidoctor/spec/elastic_compat_tree_processor_spec.rb

+  [
+    %w[CONSOLE console],
+    %w[AUTOSENSE sense],
+    %w[KIBANA kibana],


Hmmmmm...... When I wrote this I distinctly remember thinking that it was ok. On further reflection it is not OK. We use SENSE here.

nik9000 · 2019-02-26T15:05:49Z

resources/asciidoctor/spec/open_in_widget_spec.rb

+    }
+  end
+
+  %w[console sense kibana].each do |lang|


I is ok that this doesn't have autosense because this widget only support sense. autosense is transformed into sense in tree compatibility processor.

resources/website_common.xsl

And shift the warning for being hard to read to after the copy.

nik9000 · 2019-02-26T19:16:51Z

@ddillinger I switched the WARNING to an ERROR. It still keeps going, C compiler style, which is normal for asciidoctor and asciidoc. And, generally, is pretty useful even if it can make a mess sometimes.

nik9000 · 2019-03-06T16:42:56Z

Something like this begs for away to tell the user "you have 1232
warnings that you have to fix". Like linting but for your docs. I'll be
thinking more about this in the coming weeks. But for now there is not
warning when you use the compatibility layer.

I filed #671 to track this.

ddillinger

Thanks for the issue :)

estolfo

Hi Nik, I made some comments regarding minor style things and use of return. I mentioned in a comment on the rspec tests that I'm happy to discuss further in a call as a comment wouldn't be sufficient. Overall, this looks good to me. Isn't Ruby great?? ; )

estolfo · 2019-03-07T11:51:50Z

resources/asciidoctor/lib/elastic_compat_preprocessor/extension.rb

+        # CONSOLE snippet. Asciidoctor really doesn't recommend this sort of
+        # thing but we have thousands of them and it'll take us some time to
+        # stop doing it.
+        line&.gsub!(%r{//\s*(?:AUTOSENSE|KIBANA|CONSOLE|SENSE:[^\n<]+)}, 'pass:[\0]')


I would consider making the regexp a constant so that it's not instantiated each time this code is called. It also has the benefit of following this file's convention of making regexp's constants. See these lines

estolfo · 2019-03-07T11:54:40Z

resources/asciidoctor/lib/elastic_compat_tree_processor/extension.rb

-      block.subs << :specialcharacters
-      block.subs << :callouts if had_callouts
-    end
+    return unless block.context == :listing && block.style == 'source'


This is just a minor Ruby style thing, but I would typically only use return if there were a specific value I was returning when a condition was met. For example, here
In this case, I would write the code like this:

if block.context == :listing && block.style == 'source' process_subs block process_lang_override block end

estolfo · 2019-03-07T11:56:37Z

resources/asciidoctor/lib/elastic_compat_tree_processor/extension.rb

+  end
+
+  def process_subs(block)
+    return if block.subs.include? :specialcharacters


Same thing here. I would write this like:

if !block.subs.include? :specialcharacters # callouts have to come *after* special characters had_callouts = block.subs.delete(:callouts) block.subs << :specialcharacters block.subs << :callouts if had_callouts end

estolfo · 2019-03-07T12:07:23Z

resources/asciidoctor/lib/elastic_compat_tree_processor/extension.rb

+    block.set_attr 'snippet', snippet
+
+    block.parent.blocks.delete next_block
+    block.parent.reindex_sections


This might be more readable if all the logic was in one place. The same things I said above about return hold true here. I would write this code like:

def process_lang_override(block) next_block = block.next_adjacent_block unless next_block && next_block.context == :paragraph && next_block.source =~ %r{pass:\[//\s*([^:\]]+)(?::\s*([^\]]+))?\]} && lang = LANG_MAPPING[$1] snippet = $2 block.set_attr 'language', lang block.set_attr 'snippet', snippet block.parent.blocks.delete next_block block.parent.reindex_sections end end

estolfo · 2019-03-07T12:08:22Z

resources/asciidoctor/lib/open_in_widget/extension.rb

+    return unless block.context == :listing && block.style == 'source'
+
+    lang = block.attr 'language'
+    return unless %w[console sense kibana].include? lang


I would put the following in a unless %w[console sense kibana].include? lang condition.

estolfo · 2019-03-07T12:09:54Z

resources/asciidoctor/lib/open_in_widget/extension.rb

+      end
+    else
+      # If you don't specify the snippet then we assign it a number and read
+      # if from the contents of the source listing, copying it to the


I think this is a minor typo: it from the contents

estolfo · 2019-03-07T12:15:24Z

resources/asciidoctor/lib/open_in_widget/extension.rb

+    block.document.register :links, snippet_path
+
+    def block.content
+      @attributes['snippet_link'] + super


you could also write this like "#{@attributes['snippet_link']}#{super}"
It doesn't really matter, I just use string interpolation whenever I can : )

estolfo · 2019-03-07T12:23:04Z

resources/asciidoctor/spec/elastic_compat_preprocessor_spec.rb

+  WARNINGS
+  actual = convert input, stub_file_opts, eq(warnings.strip)
+  expect(actual).to eq(expected.strip)
+end


I see that your additions to this rspec file are consistent with the style of the rest of the tests but you can use let blocks and shared_examples to make it more readable and easier to extend in the future. I'm happy to discuss further in a call or in chat, as I don't think I can explain it very well in this comment.

nik9000 · 2019-03-07T15:34:08Z

Thanks @estolfo! I'm going to go through and correct a bunch of these. A few of them I absorbed from Rubocop and talking to you a few weeks ago and have grown to like. So I'm not going to change them for now and I'll let them soak into my brain for a bit first.

estolfo · 2019-03-07T15:40:54Z

They're mostly just style suggestions so take or leave what you like. Overall, looks good though!

nik9000 · 2019-03-07T15:41:56Z

They're mostly just style suggestions so take or leave what you like. Overall, looks good though!

Thanks! Mostly I've grown attached to the "early return" thing....

nik9000 · 2019-03-07T17:10:31Z

Thanks for reviewing @ddillinger and @estolfo!

ddillinger · 2019-03-07T18:23:00Z

@estolfo I sure am glad to see a proper ruby speaker in here, I've kind of just been doing the best I can :)

estolfo · 2019-03-07T20:58:10Z

I welcome any chance to nerd out about Ruby and I'm happy to help in any way that I can!

ghost · 2019-03-08T00:43:05Z

A few of them I absorbed from Rubocop

There are some really interesting ideas embodied in Rubocop, and by inference The Ruby Style Guide. This one, in particular, I think has fascinating results:

Avoid methods longer than 10 LOC (lines of code).

When I first saw that pop up in my editor, I was actually angry. "I can't believe this", I thought. "They're enforcing pretentious code-golf behaviour in the lint checker!". But, I thought I should be empirical and try doing a whole project in accordance with everything in the style guide and Rubocop...

The effect of this one particular rule was revelatory. I realised it wasn't about code-golf at all, but about so comprehensively decomposing everything that you end up "accidentally" creating a lovely little grammar for your application that consists of all these delightfully simple little functions. Trying out this rule from The Ruby Style Guide, even if I no longer apply it strictly, made me a better programmer in every language.

nik9000 · 2019-03-08T13:21:22Z

When I first saw that pop up in my editor, I was actually angry. "I can't believe this", I thought. "They're enforcing pretentious code-golf behaviour in the lint checker!". But, I thought I should be empirical and try doing a whole project in accordance with everything in the style guide and Rubocop...

The effect of this one particular rule was revelatory. I realised it wasn't about code-golf at all, but about so comprehensively decomposing everything that you end up "accidentally" creating a lovely little grammar for your application that consists of all these delightfully simple little functions. Trying out this rule from The Ruby Style Guide, even if I no longer apply it strictly, made me a better programmer in every language.

❤️

nik9000 requested review from a user and ddillinger February 21, 2019 16:36

lcawl requested a review from clintongormley February 21, 2019 17:38

nik9000 mentioned this pull request Feb 21, 2019

Asciidoctor support #505

Closed

nik9000 commented Feb 21, 2019

View reviewed changes

lcawl mentioned this pull request Feb 21, 2019

[DOCS] Document lack of support for Kibana APIs in Console elastic/kibana#25334

Closed

clintongormley reviewed Feb 21, 2019

View reviewed changes

nik9000 added 4 commits February 21, 2019 16:27

Word

b3fa0ec

Merge branch 'master' into remarks

d5fa2fb

Clean up after merge

702975e

Merge branch 'master' into remarks

5c30bbc

ddillinger reviewed Feb 26, 2019

View reviewed changes

nik9000 commented Feb 26, 2019

View reviewed changes

nik9000 added 4 commits February 26, 2019 10:07

Merge branch 'master' into remarks

85abcf4

Missing command

b74306c

speeling

e272218

Shift to error

cc8f3c7

And shift the warning for being hard to read to after the copy.

nik9000 mentioned this pull request Mar 4, 2019

Switches Stack Overview to asciidoctor builds #649

Merged

ddillinger approved these changes Mar 6, 2019

View reviewed changes

nik9000 added 2 commits March 6, 2019 12:29

Merge branch 'master' into remarks

1624996

Merge branch 'remarks' of github.com:nik9000/docs into remarks

0937803

estolfo reviewed Mar 7, 2019

View reviewed changes

Merge branch 'master' into remarks

5fe67cc

nik9000 added 2 commits March 7, 2019 10:42

Test cleanups

15ba6d2

Style

922f3ec

nik9000 merged commit 772d3c7 into elastic:master Mar 7, 2019

		@@ -1051,38 +1083,6 @@ The local web browser can be stopped with `Ctrl-C`.

		================================

		==== Custom Console snippets

Asciidoctor: Support for "open in widget" #627

Asciidoctor: Support for "open in widget" #627

Conversation

nik9000 commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Feb 26, 2019

nik9000 commented Mar 6, 2019

ddillinger left a comment

Choose a reason for hiding this comment

estolfo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Mar 7, 2019

estolfo commented Mar 7, 2019

nik9000 commented Mar 7, 2019

nik9000 commented Mar 7, 2019

ddillinger commented Mar 7, 2019

estolfo commented Mar 7, 2019

ghost commented Mar 8, 2019 • edited by ghost Loading

nik9000 commented Mar 8, 2019

ghost commented Mar 8, 2019 •

edited by ghost

Loading