Asciidoctor: Fix Elastic's include-tagged with empty lines #693
Conversation
Elastic has has tagged include support in AsciiDoc for years now. Once
we've migrated to Asciidoctor we can start using its native tagged
include support, but while we're in the process of migrating we need to
perfectly emulate Elastic's tagged include in Asciidoctor. We've had
this emulation for a while and this change fixes a bug which caused
empty lines to be throw out. Let's look at an example:
Say you have some file like this:
```
...
// tag::foo
String foo = "bar";
System.err.println("ASDFDAF " + foo);
// end::foo
...
```
Elastic's AsciiDoc include tagged support would extract the `foo` tag
as:
```
String foo = "bar";
System.err.println("ASDFDAF " + foo);
```
The bug in our Asciidoctor include tagged support is that it'd extract
the `foo` tag as:
```
String foo = "bar";
System.err.println("ASDFDAF " + foo);
```
Those empty lines can be quite useful in examples! We should preserve
them.
nik9000
changed the title
|
@estolfo, could you have a look at this one when you get a chance? |
| start_match = /^(\s*).+tag::#{tag}\b/ | ||
| end_match = /end::#{tag}\b/ | ||
| start_match = /^(\s*).+tag::#{tag}\b/.freeze | ||
| end_match = /end::#{tag}\b/.freeze |
There was a problem hiding this comment.
Is there a specific reason why you are freezing the Regexp's?
I usually freeze Strings when I use them as constants, as they are mutable. Things like Regexp's, that are immutable, are probably not necessary to freeze.
There was a problem hiding this comment.
It looks like regexes are technically mutable even if they don't have any mutation methods. Weird. I suspect some of this weirdness is how I got here. I'll stop freezing them.
| included_lines << line if line | ||
| next | ||
| end | ||
| next unless start_match =~ line | ||
| start_match_data = start_match.match(line) | ||
| next unless start_match_data |
There was a problem hiding this comment.
You could also write this like next unless start_match_data = start_match.match(line)
But some people might disagree : )
There was a problem hiding this comment.
Dan didn't like that too much so I'm avoiding it. Asciidoctor uses this sort of thing in the extreme and I think it is fairly hard to read. So for the most part I've swung the other way....
There was a problem hiding this comment.
It's purely a style thing, so no problem
|
|
||
| found_tag = true | ||
| indentation = $1.size | ||
| indentation = /^#{start_match_data[1]}/.freeze |
There was a problem hiding this comment.
Regexp's are immutable so it's probably not necessary to freeze it.
|
@estolfo this one is ready for you again when you get a chance. |
Elastic has has tagged include support in AsciiDoc for years now. Once
we've migrated to Asciidoctor we can start using its native tagged
include support, but while we're in the process of migrating we need to
perfectly emulate Elastic's tagged include in Asciidoctor. We've had
this emulation for a while and this change fixes a bug which caused
empty lines to be throw out. Let's look at an example:
Say you have some file like this:
Elastic's AsciiDoc include tagged support would extract the
footagas:
The bug in our Asciidoctor include tagged support is that it'd extract
the
footag as:Those empty lines can be quite useful in examples! We should preserve
them.