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

Fix emitting :: for centered column in table. #76

Merged
merged 1 commit into from
Jun 16, 2024
Merged

Fix emitting :: for centered column in table. #76

merged 1 commit into from
Jun 16, 2024

Conversation

Ethiraric
Copy link
Contributor

Emitting :: in a table to indicate alignment is Center seems to break some parsers (at least mdbook's). Emitting :-: appears to work fine.

This commit also removes an allocation when encountering an empty header cell. Rather than allocating a non-empty string, let the string be empty and add a minimum width for emitting alignments.

Also, I replaced what I think is a mismatch between characters and bytes in:

let last_minus_one = name.chars().count().saturating_sub(1);
for c in 0..name.len() {

last_minus_one counts in chars, while c counts in bytes.

This might fix #70.

Emitting `::` in a table to indicate alignment is `Center` seems to
break some parsers. Emitting `:-:` appears to work fine everywhere.

This commit also removes an allocation when encountering an empty header
cell. Rather than allocating a non-empty string, let the string be empty
and add a minimum width for emitting alignments.
@Ethiraric
Copy link
Contributor Author

Looking more into #70, this definitely fixes it.

The issue is that the column has a single-character heading. The unwrap_or(" ".into) is not executed, and the width of the column is set to 1. A width of 1 is too small to express centering (whether it be :-: or ::) and the emitter emits a single : for the alignment.

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

This is wonderful, thanks so much!

I love the extra test as well, even though I admit that I am not very familiar with the table syntax and trust that it will render fine as well.

@Byron Byron merged commit 2ac7472 into Byron:main Jun 16, 2024
1 check passed
@Byron
Copy link
Owner

Byron commented Jun 16, 2024

let last_minus_one = name.chars().count().saturating_sub(1);
for c in 0..name.len() {

I forgot to comment on this: That's a great catch, and averts a disaster that was just waiting to happen.

And here is the new release: https://github.com/Byron/pulldown-cmark-to-cmark/releases/tag/v15.0.1 .

@Ethiraric
Copy link
Contributor Author

Not a lot has changed in the output. In any case, this fixes my output for centered columns.

The only change I made to your test is to remove a - in the yo column. This means alignment is written as a single - as opposed to 2.

With empty columns, the emitter did always output 2 characters. This is what is needed for left and right alignment, one more than needed for default alignment (but it causes no error), but one too few for the centered alignment.

A side effect of my change, which fixed #70, is that the minimum width requirement is independent from the length of the header column. If you had the following:

|y|
|:-:|

It would have output

|y|
|:|

@Ethiraric
Copy link
Contributor Author

And here is the new release: https://github.com/Byron/pulldown-cmark-to-cmark/releases/tag/v15.0.1 .

Wow, I did not expect to have a release so soon! Thanks! You save me from having a temporary workaround <3

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.

table it_generates_equivalent_table_markdown lost alignments
2 participants