Asciidoctor: Copy referenced images #541
Conversation
Mimicks `a2x`'s behavior with regards to images, copying them into the destionation repository. `a2x` parses the generated html files but we don't want to use their parser because we'd like to make a clean break from `a2x`. We can't use any of perl's wonderful parsers because we don't want to add any additional dependencies to the project. Instead, we use the AST of the parsed asciidoc files and look for images. This isn't *quite* the same thing because it'll miss any images that we add using as part of the xsl transformation but we can handle those explictly in the perl code if there are any. In addition to not requiring any more dependencies it has the advantage of build "built in" to the asciidoctor work so we don't need to work on it later if we change the perl orchestration stuff. Finally, it is pretty quick because we don't have to parse all of the generated html - we can just use the already parsed asciidoctor!
| @@ -0,0 +1,69 @@ | |||
| require 'csv' | |||
There was a problem hiding this comment.
We have to parse a comma separated list and the simplest way looks to be to use the built in CSV parsing....
| return unless @copied.add? uri # Skip images we've copied before | ||
| source = find_source block, uri | ||
| return unless source # Skip images we can't find | ||
| logger.info message_with_context "copying #{uri}", :source_location => block.source_location |
There was a problem hiding this comment.
I'm not sure if info is right here but it doesn't seem too bad. We don't have too many images.
| "#{spec_dir}\/not_found.png",\s | ||
| "#{tmp}\/not_found.png",\s | ||
| "\/dummy2\/not_found.png" | ||
| .+ |
There was a problem hiding this comment.
We list out all of the directories that we try and I don't think that list is worth maintaining here. I do think it is helpful to see it when it fails. Though I don't like how long the list gets.....
|
|
||
| private | ||
| def copy_attributes copied | ||
| { |
There was a problem hiding this comment.
Is the funky formatting meant to communicate something here?
There was a problem hiding this comment.
We're returning a map. Would it be clearer if I said return {? I don't ruby too well so I'm not sure.
There was a problem hiding this comment.
I think it would avoid confusion with things like block formatting. It makes the reader have to think about whether there are any | | present or something.
| image::resources/copy_images/example1.png[] | ||
| ASCIIDOC | ||
| expect { convert input, attributes }.to raise_error { |error| | ||
| expect(error).to be_a(ConvertError) |
There was a problem hiding this comment.
I found this testing idiom unintuitive. By description this appears to be a positive test, one which should do a thing (and behave doing so) and then verify the thing has been done. But this is looking for an error, and then that error is actually an INFO... it's got a lot to tease apart just to line up what it says it does with how it checks doing it.
I don't know yet if I am requesting a change or not, but even if I figure there's probably a really good explanation for it, I'd still like to hear it because it definitely made me go "...huh?" reviewing it.
There was a problem hiding this comment.
It exists because the tests throw a ConvertError if we log anything during the conversion. That way when we're testing that we don't log anything we don't have to do anything - we get that behavior by not catching the exception. It made more sense when I was catching warnings to be honest.
I think I can make this more clear. I'll give it a shot on Monday.
|
@ddillinger I've merged master into this to fix the dependency issue and cleaned up the error reporting. |
|
|
||
| private | ||
| def copy_attributes copied | ||
| { |
There was a problem hiding this comment.
I think it would avoid confusion with things like block formatting. It makes the reader have to think about whether there are any | | present or something.
|
Thanks for reviewing @ddillinger! I've added the |
Mimicks `a2x`'s behavior with regards to images, copying them into the destionation repository. `a2x` parses the generated html files but we don't want to use their parser because we'd like to make a clean break from `a2x`. We can't use any of perl's wonderful parsers because we don't want to add any additional dependencies to the project. Instead, we use the AST of the parsed asciidoc files and look for images. This isn't *quite* the same thing because it'll miss any images that we add using as part of the xsl transformation but we can handle those explictly in the perl code if there are any. In addition to not requiring any more dependencies it has the advantage of build "built in" to the asciidoctor work so we don't need to work on it later if we change the perl orchestration stuff. Finally, it is pretty quick because we don't have to parse all of the generated html - we can just use the already parsed asciidoctor!
Mimicks
a2x's behavior with regards to images, copying them into thedestionation repository.
a2xparses the generated html files but wedon't want to use their parser because we'd like to make a clean break
from
a2x. We can't use any of perl's wonderful parsers because wedon't want to add any additional dependencies to the project. Instead,
we use the AST of the parsed asciidoc files and look for images. This
isn't quite the same thing because it'll miss any images that we add
using as part of the xsl transformation but we can handle those
explictly in the perl code if there are any. In addition to not
requiring any more dependencies it has the advantage of build "built in"
to the asciidoctor work so we don't need to work on it later if we
change the perl orchestration stuff. Finally, it is pretty quick because
we don't have to parse all of the generated html - we can just use the
already parsed asciidoctor!