Asciidoctor: BWC for // CONSOLE then callouts #721

nik9000 · 2019-03-15T14:26:41Z

Our backwards compatibility support for "OPEN IN CONSOLE"-style links did
not work with snippets followed by a callout list. That looks like this:

[source,js]
----
GET / <1>
----
// CONSOLE
<1> words go here

These is actually super common and I'm surprised I didn't catch it
before! This adds tests for it so it won't sneak through again.

The reason this didn't work is that our backwards compatibility worked by
transforming the // CONSOLE comment in pass:[// CONSOLE] and then
catching that when walking the tree. We do this in two passes because our
preprocessor works line by line and reading ahead can cause issues. So we
do the "action at a distance" in a tree processor where it is quite
simple.

The trouble is that the pass:[// CONSOLE] construct is inline so when
it is immediately followed by the <1> words go here line it gets sucked
into the same block which Asciidoctor thinks of as a paragraph. This
causes the parsing for the callout list not to happen, causing all kinds
of trouble.

The fix for this is to introduce a block macro that functions like
pass:[// CONSOLE] and to use that instead. This causes the callout list
to be parsed properly.

Our backwards compatibility support for "OPEN IN CONSOLE"-style links did not work with snippets followed by a callout list. That looks like this: ``` [source,js] ---- GET / <1> ---- // CONSOLE <1> words go here ``` These is actually *super* common and I'm surprised I didn't catch it before! This adds tests for it so it won't sneak through again. The reason this didn't work is that our backwards compatibility worked by transforming the `// CONSOLE` comment in `pass:[// CONSOLE]` and then catching that when walking the tree. We do this in two passes because our preprocessor works line by line and reading ahead can cause issues. So we do the "action at a distance" in a tree processor where it is quite simple. The trouble is that the `pass:[// CONSOLE]` construct is *inline* so when it is immediately followed by the `<1> words go here` line it gets sucked into the same block which Asciidoctor thinks of as a paragraph. This causes the parsing for the callout list not to happen, causing all kinds of trouble. The fix for this is to introduce a block macro that functions like `pass:[// CONSOLE]` and to use that instead. This causes the callout list to be parsed properly.

estolfo

Overall, looks good. I personally like to put newlines in between rspec blocks so I can visualize the contexts. But that's up to you!

estolfo · 2019-03-15T17:57:28Z

resources/asciidoctor/lib/elastic_compat_tree_processor/extension.rb


-    lang = LANG_MAPPING[$1]
-    snippet = $2
+    m = %r{^//\s*([^:\]]+)(?::\s*([^\]]+))?$}.match(next_block.source)


I think a longer variable name would make this clearer.

estolfo · 2019-03-15T17:58:37Z

resources/asciidoctor/lib/lang_override/extension.rb

+
+##
+# Block macro that exists entirely to mark language overrides in a clean way
+# without additing additional lines to the input file which would throw off


without adding additional

estolfo · 2019-03-15T18:01:23Z

resources/asciidoctor/spec/elastic_compat_tree_processor_spec.rb

+          ----
+        ASCIIDOC
+        snippet += "lang_override::[#{override}]" if override
+        snippet


Have you seen the #tap method in Ruby? It's a style thing, but you could use it here. I like using it so I never accidentally forget to return the object (in this case, snippet) from the let block.

#tap seems pretty complex to use here because the string is frozen. Though I bet there are other places i can use it....

ah, then it wouldn't work if the String is frozen..

estolfo · 2019-03-15T18:18:44Z

resources/asciidoctor/spec/elastic_compat_tree_processor_spec.rb

+            #{snippet}
+          ASCIIDOC
+        end
+        include_examples 'has the expected language'


This is very nuanced, but you could also use it_behaves_like here. include_examples is best suited for what you're doing from the topic level - creating specs using different values but when you have a nested context and want to emphasize a behavior, people tend to use it_behaves_like. They're effectively interchangeable and I think the difference really comes down to how you want the specs to print out. Sometimes it's also awkward to find a phrase that describes the behavior. For example, in this case, it might be strange to say it_behaves_like 'a string converter' - or whatever. So anyway, maybe include_examples is best here and you can ignore this whole comment = )

Thanks for this! I looked at it_behaves_like for a bunch of things. I'll play with it some more and see what I can find. I found that it doesn't work well when you want to apply the same set of shared examples multiple times with different parameters. Or, rather, it doesn't seem to properly list the parameters when you run rspec. I'll try it some more though.

I see what you're saying. Ultimately, they can accomplish very similar things in the specs, so it's up to you and how you want the the specs to run and print out.

nik9000 · 2019-03-15T14:27:53Z

resources/asciidoctor/lib/elastic_compat_preprocessor/extension.rb

@@ -113,7 +113,7 @@ class ElasticCompatPreprocessor < Asciidoctor::Extensions::Preprocessor
  INCLUDE_TAGGED_DIRECTIVE_RX = /^include-tagged::([^\[][^\[]*)\[(#{Asciidoctor::CC_ANY}+)?\]$/
  SOURCE_WITH_SUBS_RX = /^\["source", ?"[^"]+", ?subs="(#{Asciidoctor::CC_ANY}+)"\]$/
  CODE_BLOCK_RX = /^-----*$/
-  SNIPPET_RX = %r{//\s*(?:AUTOSENSE|KIBANA|CONSOLE|SENSE:[^\n<]+)}
+  SNIPPET_RX = %r{^//\s*(AUTOSENSE|KIBANA|CONSOLE|SENSE:[^\n<]+)$} # NOCOMMIT handle trailing spaces


I'll get this before merging!

nik9000 · 2019-03-15T18:24:05Z

resources/asciidoctor/spec/elastic_compat_tree_processor_spec.rb

+            #{snippet}
+          ASCIIDOC
+        end
+        include_examples 'has the expected language'


Thanks for this! I looked at it_behaves_like for a bunch of things. I'll play with it some more and see what I can find. I found that it doesn't work well when you want to apply the same set of shared examples multiple times with different parameters. Or, rather, it doesn't seem to properly list the parameters when you run rspec. I'll try it some more though.

nik9000 · 2019-03-15T20:06:20Z

OK! I've pushed some more tests for the regex on the way in. I think this is ready for review again!

estolfo

Looks good!

estolfo · 2019-03-26T12:50:16Z

resources/asciidoctor/spec/elastic_compat_tree_processor_spec.rb

+          ----
+        ASCIIDOC
+        snippet += "lang_override::[#{override}]" if override
+        snippet


ah, then it wouldn't work if the String is frozen..

estolfo · 2019-03-26T12:53:21Z

resources/asciidoctor/spec/elastic_compat_tree_processor_spec.rb

+            #{snippet}
+          ASCIIDOC
+        end
+        include_examples 'has the expected language'


I see what you're saying. Ultimately, they can accomplish very similar things in the specs, so it's up to you and how you want the the specs to run and print out.

nik9000 · 2019-03-26T18:13:08Z

Thanks for reviewing @lcawl!

nik9000 · 2019-03-26T18:14:38Z

Thanks for reviewing @lcawl!

Wow, that is embarrassing. "I meant thanks for reviewing @estolfo!" I mean, both of you review my code but for different reasons and I was just sending something to @lcawl.... But I'm not sure about why I mixed you two up there. Sorry!

nik9000 requested a review from estolfo March 15, 2019 14:26

estolfo reviewed Mar 15, 2019

View reviewed changes

nik9000 commented Mar 15, 2019

View reviewed changes

nik9000 added 4 commits March 15, 2019 14:34

Merge branch 'master' into asciidoctor_lang_override_callouts

a830f1d

Longer name

96bc662

More tests

97f882c

test

b55d932

nik9000 marked this pull request as ready for review March 15, 2019 20:04

Words

73e73d0

This was referenced Mar 15, 2019

Switches Stack Overview to asciidoctor builds #649

Merged

Switches X-Pack Reference to Asciidoctor builds #653

Merged

estolfo approved these changes Mar 26, 2019

View reviewed changes

nik9000 merged commit 29e8fcd into elastic:master Mar 26, 2019

Asciidoctor: BWC for // CONSOLE then callouts #721

Asciidoctor: BWC for // CONSOLE then callouts #721

Uh oh!

Conversation

nik9000 commented Mar 15, 2019

Uh oh!

estolfo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Mar 15, 2019

Uh oh!

estolfo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Mar 26, 2019

Uh oh!

nik9000 commented Mar 26, 2019

Uh oh!

Uh oh!