Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asciidoctor images #6671

Merged
merged 11 commits into from
Sep 20, 2020
Merged

Asciidoctor images #6671

merged 11 commits into from
Sep 20, 2020

Conversation

argent0
Copy link
Contributor

@argent0 argent0 commented Sep 8, 2020

Addresses #6538 by implementing jgm's proposed solution.

I had to change writer.asciidoctor to pass the tests.

Concretely:

Given the file: my-test.markdown

Inline-style: ![alt text](media/rId25.jpg)

Block image:


![alt text 2](media/rId26.jpg "image title")

The end.

This patch generates, after running:

$ stack run pandoc -- -f markdown test/my-test.markdown -t asciidoctor -o -

The following file:

Inline-style: image:media/rId25.jpg[alt text]

Block image:

image::media/rId26.jpg[alt text 2,title="image title"]

The end.

I think the current basic test doesn't distinguish between inline and block figures. Should the distinction be added to the basic test?

As mentioned in #3177, this fix is brittle for the time being.

I have only just seen kukimik's comment.

@argent0
Copy link
Contributor Author

argent0 commented Sep 8, 2020

I see the asciidoc tests failing. According to https://asciidoctor.org/docs/asciidoc-syntax-quick-reference/#images
this is valid asciidoc syntax. So I will update the asciidoc test too.

Block images generate an `image::` line.
Copy link
Collaborator

@tarleb tarleb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution, the result of this looks good. Below I list are a few things that came to mind, in no particular order.

Maybe checkout the type classe IsString, of which Doc Text is an instance. It allows to write a value as if it was a string. (E.g. Text 3 "foo" can be shortened to just "foo").

Pattern matching on the specific Doc constructors is clever, as it produces very compact code. But it is also fragile and would break if the return value changed, e.g. by changing the order in which values are concatenated. It would be more robust to construct the result directly. One option could be to construct a new image helper function which both blockToAsciiDoc and inlineToAsciiDoc can use.

It's used by both inline an block images.
@argent0
Copy link
Contributor Author

argent0 commented Sep 9, 2020

I implemented @tarleb 's proposed refactoring of the common image code into a helper function that is called by blockToAsciiDoc & inlineToAsciiDoc.

I've run into some trouble though. The command

$ stack run pandoc -- -f markdown test/my-test.markdown -t asciidoctor -o -

generates:

Inline-style: image:media/rId25.jpg[alt text]                                                      
                                                                                                   
Block image:                                                                                       
                                                                                                   
image::media/rId26.jpg[alt text 2,title="image title"]Another paragraph.                           
                                                                                                   
The end.  

Notice the: "Another paragraph" at the the end of the block image.

Yet, the native output seems fine:

$ stack run pandoc -- -f markdown test/my-test.markdown -t native -o -
[Para [Str "Inline-style:",Space,Image ("",[],[]) [Str "alt",Space,Str "text"] ("media/rId25.jpg","
")]                                                                                                
,Para [Str "Block",Space,Str "image:"]                                                             
,Para [Image ("",[],[]) [Str "alt",Space,Str "text",Space,Str "2"] ("media/rId26.jpg","fig:image ti
tle")]                                                                                             
,Para [Str "Another",Space,Str "paragraph."]                                                       
,Para [Str "The",Space,Str "end."]]  

As the "Another paragraph" is in its own Para. 🤔

This is the input file my-test.markdown:

Inline-style: ![alt text](media/rId25.jpg)

Block image:


![alt text 2](media/rId26.jpg "image title")


Another paragraph.

The end.

Any idea, 1) if this is an issue; 2) where is the code to solve it?

Regards

@tarleb
Copy link
Collaborator

tarleb commented Sep 9, 2020

Great, looking good! I believe it should be enough to append a blankline to the result in the block. Similar to what happens in the last line of blockToAsciiDoc for Para elements.

@tarleb
Copy link
Collaborator

tarleb commented Sep 10, 2020

Almost ready to merge, just one tiny nitpick remaining: would you break the long lines 144 and 559 such that they have 80 chars or less? We try to keep all source lines below that limit. (I'm aware that 559 was overlong before, so it may as well stay that way. But since we are fixing things anyway...)

I think it could also be clearer to use Attr instead of (Text, [Text], [(Text, Text)]) in the type signature.

Copy link
Collaborator

@tarleb tarleb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me!

Leaving it to @jgm to double-check and merge.

@argent0
Copy link
Contributor Author

argent0 commented Sep 10, 2020

Thank you for all the help @tarleb.

@jgm jgm merged commit ba9bede into jgm:master Sep 20, 2020
@jgm
Copy link
Owner

jgm commented Sep 20, 2020

Looks good, thanks for the PR!

@argent0 argent0 deleted the asciidoctor-images branch September 21, 2020 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants