-
-
Notifications
You must be signed in to change notification settings - Fork 170
Fix table-short-captions support for Pandoc 2.10+ #149
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
Conversation
Add the -f flag to rm to avoid "no such file" messages and the non-zero exit code.
@blake-riley please review. |
Agh, it works with every Pandoc version I tried on my Mac. I need to figure out why the CI is failing. |
The new tests introduced by 5466859 (table-short-captions: replace golden files with expected strings, 2019-05-10) have been broken all this time. They always pass because they actually do not check anything. The assert_contains function receives the test string to be searched on stdin, but inside the function it is wrongly assumed to be in $1, which is not set. Thus, grep searches for the empty string and all the tests always pass. Fix by consuming stdin inside the function to get the content to check, which ensures the content is given as the query to grep.
The generated output was changed by by 7a49118 (Fix #48 by using \let=\relax and \@ifundefined, 2019-03-31).
Commit 6e1b2cc (table-short-captions: add support for pandoc 2.10, 2020-07-15) doesn't work because it treats the new `Table.caption.long` object as the pre 2.10 `Table.caption` object, but they are completely different lists. `Table.caption.long` is a list of `Blocks`, where each block has a `content` key which is a `List` of inlines (or it should be, a test would be needed for more complex captions?). The pre 2.10 `Table.caption` is a `List` of inlines, so they cannot share the same code to be manipulated. Fix by just splitting into two different functions. They do follow the same structure, but it is better to keep them separated, otherwise it would require too many ifs, or code that is not clear. For pre 2.10 `Table.caption` just keep the code as it is. For the new `Table.caption.long`, ensure that the outer list of `Blocks` is taken into account when searching for the inline `Span` with the short caption. Now all tests pass again.
Rebased to fix the CI ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you! I left one inline comment, but otherwise this is ready to be merged.
local label, short_caption, unlisted = parse_table_attrs(propspan.attr) | ||
|
||
-- Excise the span from the caption | ||
caption[bidx].content[idx] = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this truncate the caption if #caption[bidx] ~= idx
? How about
caption[bidx].content[idx] = #caption[bidx] ~= idx and pandoc.Span{} or nil
Alternatively, we could also expect the properties span to always be the last element in the last block, which would then simplify find_properties
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the original code, which uses
caption[idx] = nil
It would cause the same issue if #caption ~= idx
in that case, but I didn't want to refactor the filter too much.
I guess the properties span will always appear at the end (it is expecting the attributes to be converted to span by adding the []
, and the attributes always are at the end).
Maybe another PR to not mix too much changes in this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's very reasonable.
A separate PR would be welcome!
Support for Pandoc 2.10 is not working, and all the tests are broken too, they do not check anything.
This PR fixes Pandoc 2.10 support, and also fixes the tests to ensure they do run (and updates them to the changes introduced by 7a49118).
Commit 6e1b2cc introduced support for Pandoc 2.10 but it doesn't work because it treats the new
Table.caption.long
object as the pre 2.10Table.caption
object, but they are completely different lists.Table.caption.long
is a list ofBlocks
, where each block has acontent
key which is aList
of inlines (or it should be, a test would be needed for more complex captions?). The pre 2.10Table.caption
is aList
of inlines, so they cannot share the same code to be manipulated.This PR fixes it by just splitting the code into two different functions. They do follow the same structure, but it is better to keep them separated, otherwise it would require too many ifs, or code that is not clear.
For pre 2.10
Table.caption
just keep the code as it is. For the newTable.caption.long
, ensure the outer list ofBlocks
is taken into account when searching for the inlineSpan
with the short caption.Now all test do run and all pass, with pre and post Pandoc 2.10 (
nix-run-version
is a helper that allows me to use nixpkgs releases to run different versions of a command)Before this PR (well, not really, this PR also fixes the tests to see them failing. Before the last commit fixing 2.10 support):
With this PR: