Asciidoctor: Copy images for inline image macro #736
Conversation
This copies images declared "inline". Asciidoctor doesn't give a good way of hooking into these images so we have to scan for them when we walk the tree. It isn't the best solution, but at least we don't have to scan for them after the document is rendered! If we did that we wouldn't get line number information when we can't find images.... Closes elastic#730
|
|
||
| ## | ||
| # Convert an asciidoc string into docbook returning the result and any warnings. | ||
| def convert_with_logs(input, extra_attributes = {}) |
There was a problem hiding this comment.
This seems like a much more rspec friendly way to check for logs. Though I still like having two flavors - one that automatically asserts that there weren't any logs so we don't have to have it didn't log anything assertions all over the place....
There was a problem hiding this comment.
Maybe you could add an expectation using allow or expect. That way, you won't get an assertion it didn't log anything
There was a problem hiding this comment.
Oooh! Let me do some research there.
| include_examples 'copies images' | ||
| end | ||
| context 'for the image inline macro' do | ||
| # NOCMOMIT rename image_ref to target |
There was a problem hiding this comment.
Woops. Looks like eI need to pick up these nocommits
| end | ||
|
|
||
| def process_image(block, uri) | ||
| return if uri == '' |
There was a problem hiding this comment.
is there any chance uri could be nil?
There was a problem hiding this comment.
Not really, but I'll do return unless uri just because that feels a little more "ruby" to me.
| @@ -6,7 +6,7 @@ | |||
| require 'fileutils' | |||
| require 'tmpdir' | |||
|
|
|||
| RSpec.describe CopyImages::CopyImages do | |||
| RSpec.describe CopyImages do | |||
There was a problem hiding this comment.
you can also just write describe without calling it on Rspec
There was a problem hiding this comment.
That causes an undefined method error for me.
There was a problem hiding this comment.
hmm, maybe your version of RSpec is older
Anyway, it doesn't really matter how you call describe
| ]) | ||
| end | ||
| it 'logs that it copied the image' do | ||
| expect(logs).to eq("INFO: <stdin>: line #{include_line}: copying #{spec_dir}/resources/copy_images/example1.png") |
There was a problem hiding this comment.
The expected result is a bit long, so perhaps it can be put in a let block so that it's easier to parse. Also, if the expected result changes, it's easier to locate in the spec file what needs to edited.
For example:
let(:expected_copied_image) do
[[resolved, "#{spec_dir}/resources/copy_images/example1.png"],]
end
it 'logs that it copied the image' do
expect(logs).to eq(expected_copied_image)
end|
|
||
| ## | ||
| # Convert an asciidoc string into docbook returning the result and any warnings. | ||
| def convert_with_logs(input, extra_attributes = {}) |
There was a problem hiding this comment.
Maybe you could add an expectation using allow or expect. That way, you won't get an assertion it didn't log anything
| result | ||
| end | ||
| let(:logs) { result[1] } | ||
| let(:copied) { result[2] } |
There was a problem hiding this comment.
I find this to be a little confusing. I think the variables you're interested in can be extracted out into their own let blocks and then the ! can be used to ensure that the result is created before other variables are accessed. I'm not sure if this would work, but I'm thinking something like this that isolates the variables.
let!(:logs) do
attributes = copy_attributes copied
(convert_with_logs input, attributes)[1]
end
let(:copied) do
[]
endIs the copied object mutated by the copy_attributes method?
|
@estolfo, I pushed something to switch from the tuple-returning method to a shared context that can be included that defines the things that I might assert on. I haven't done anything about logging so if there aren't any logging assertions then logs are ignored, but that seems fine for this change. I can handle it in a follow up I think. |
| .map { |l| "#{l[:severity]}: #{l[:message].inspect}" } | ||
| .join("\n") | ||
| end | ||
| end |
There was a problem hiding this comment.
yes! This looks good and more rspec-y
|
Thanks for reviewing @estolfo! |
This copies images declared "inline". Asciidoctor doesn't give a good
way of hooking into these images so we have to scan for them when we
walk the tree. It isn't the best solution, but at least we don't have to
scan for them after the document is rendered! If we did that we wouldn't
get line number information when we can't find images....
Closes #730