search

riboseinc / asciidoctor-bibliography

Citations in AsciiDoc
MIT License
30 stars 8 forks source link

Fix 78 #96

Closed paolobrasolin closed 5 years ago

paolobrasolin commented 5 years ago

@ronaldtse This was a bit more subtle than it appeared, but I think we're done.

I caught and documented the solution the only edge case I found which might surprise users (custom citation text using ]). It's due to the asciidoctor parser, though, so it might be common knowledge.

@andrewcarver, @pela9 and @oncletom in case you're able to give this a spin, let me know whether this solves #78 in your use cases. Thanks!

ronaldtse commented 5 years ago

@paolobrasolin the ] is really strange. I realize that it is necessary for prefixes, but is there another solution for this?

paolobrasolin commented 5 years ago

@ronaldtse, asciidoctor behaviour is:

# work as expected
1. xref:fookey[b[a\]ar]
2. <<fookey,b[a]ar>>

# actually prints \
3. xref:fookey[b[a\\]ar]
4. <<fookey,b[a\]ar>>

# macro truncated at first ]
5. xref:fookey[b[a]ar]

Reproducing this behaviour (mandatory escape as \]) definitely is better for the user.

However I went with &rsqb; because that worked both inside and outside footnotes effortlessly.

Let me explain: inside footnotes the only working option is 4.

This is a bit problematic: internally, we're replacing

cite:target[options]            -> <<target,options>>
footnote:[cite:target[options]] -> footnote:[<<target,options>>]

The first case works if options are not escaped, the second one works it options are escaped.

A reasonable choice would be to require escaping (since 1 is the same kind of macro syntax-wise and it is expected to work). This would require unescaping them when inside a footnote, and in turn this means that asciidoctor-bibliography needs to be aware of where the citation is placed (all of this is unnecessary if using &rsqb;).

Given we're in a preprocessor (no document AST available) this would require some further parsing/accounting for footnote.

I'm trying to come up with an alternative solution, but I'm not seeing one right now. Any thoughts?

ronaldtse commented 5 years ago

@paolobrasolin Indeed without being able to hook into asciidoctor we won't be able to fix this issue. Let's just merge this for now if you're good with it. Thanks!