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

Support fixed-width table alignment row #217

Open
vweevers opened this issue Aug 31, 2019 · 14 comments
Open

Support fixed-width table alignment row #217

vweevers opened this issue Aug 31, 2019 · 14 comments
Labels
🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have

Comments

@vweevers
Copy link

Subject of the feature

Support fixed-width columns like | --- | --- | regardless of cell content.

Problem

I'm looking to match remark-lint-table-cell-padding with the following remark settings:

settings: {
  spacedTable: true,
  paddedTable: false,
  stringLength: () => 3 // Or 0 (has the same effect in markdown-table)
}

The use case is tables with long cell content, for example in standard and Level/awesome. In these cases, the 'padded' option of remark-lint-table-cell-padding creates too much (git diff) noise. But the 'compact' option makes these big tables difficult to read.

I propose to add a 'fixed' option. Is that feasible?

Expected behaviour

This should be valid:

| foo | a long column | bar |
| --- | --- | --- |

Missing spaces or a length that isn't 3 should be invalid:

|foo | a long column | bar|
|--- | ------- | -|
@vweevers vweevers added 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Aug 31, 2019
vweevers added a commit to vweevers/hallmark that referenced this issue Aug 31, 2019
Because on big tables it creates noise.

Setting "paddedTable" to "false" in package.json does 3 things:

1. Disable padding by `remark-stringify`
2. Use fixed width columns: `| --- | --- |`
3. Disable `remark-lint-table-cell-padding`

Linting is disabled because `remark-lint-table-cell-padding` has
no option (yet) for this particular style.

See: remarkjs/remark-lint#217

Prior discussion: #16
@wooorm
Copy link
Member

wooorm commented Aug 31, 2019

🤔 I think this is unrelated to table-cell-padding, as this rule checks whether there’s a space around | or not.
It already, when given padded, treats the valid example as valid, and warns about the invalid example.

Isn’t this closed to table-pipe-alignment?

Btw, if I recall correctly, the table rules don’t check the alignment row at all 🤔

@vweevers
Copy link
Author

and warns about the invalid example

I should have given a more specific invalid example:

| foo | a long column | bar |
| --- | ------------- | --- |

@vweevers
Copy link
Author

Isn’t this closed to table-pipe-alignment?

Its readme states:

You can pass your own stringLength function to customize how cells are aligned. In that case, this rule must be turned off.

So it seems there's currently no way to lint this.

@wooorm
Copy link
Member

wooorm commented Aug 31, 2019

If I understand correctly, you’d like for the following to be valid:

| foo | a long column | bar |
| --- | --- | --- |
| alpha | bravo | charlie |

...right? So this rule/option/feature is only about the alignment row (| --- | --- | --- |)?

@wooorm
Copy link
Member

wooorm commented Aug 31, 2019

Isn’t this closed to table-pipe-alignment?

Right, but that rule is for the padding, e.g., warning if |a or b| is used, instead of | a and b |, so it would warn for your original invalid example (albeit not on the alignment row)

@vweevers
Copy link
Author

...right? So this rule/option/feature is only about the alignment row

Right. I didn't know that the "alignment row" had a name, cool.

Does it warrant a separate plugin?

@vweevers vweevers changed the title remark-lint-table-cell-padding: support fixed-width columns Support fixed-width table alignment row Aug 31, 2019
@wooorm
Copy link
Member

wooorm commented Aug 31, 2019

I don’t think it makes sense to have fixed-width on normal cells, only for the alignment row, sooo, yeah, I think a new rule makes sense...

Maybe it’s time to revisit how the alignment row is created when not aligning cells.
Ideally remark would be able to create this table style.

What’s the reason for three? Why not two? One? Four?

@vweevers
Copy link
Author

Maybe it’s time to revisit how the alignment row is created when not aligning cells.
Ideally remark would be able to create this table style.

FWIW when I discovered the paddedTable option I expected that setting it to false would affect both cells (no padding with spaces) and the alignment row (no padding with dashes).

What’s the reason for three? Why not two? One? Four?

Because three is currently the minimum of markdown-table. Consequently, that's the minimum of remark-stringify, even when you pass stringLength: () => 0. Maybe that's worth revisiting - but personally I don't care whether it's one, two or three, I care more about consistency.

@wooorm
Copy link
Member

wooorm commented Aug 31, 2019

FWIW when I discovered the paddedTable option I expected that setting it to false would affect both cells (no padding with spaces) and the alignment row (no padding with dashes).

The paddedTable option in remark is about |a and b| (when false) and | a and b | (when true). It does this for the alignment row as well. But that’s different than having a fixed length.

Because three is currently the minimum of markdown-table. Consequently, that's the minimum of remark-stringify, even when you pass stringLength: () => 0. Maybe that's worth revisiting - but personally I don't care whether it's one, two or three, I care more about consistency.

Three is historically the lowest-possible alignment row cell length. Apparently GFMs support is now, after switching to be based upon CM, different:

Say this Markdown was used:

| 1 dash |
| - |
| a |

| 1 colon |
| : |
| a |

| two colons |
| :-: |
| a |

| colon dash |
| :- |
| a |

| dash colon |
| -: |
| a |

| colon dash colon |
| :-: |
| a |

Yields:

1 dash
a

| 1 colon |
| : |
| a |

two colons
a
colon dash
a
dash colon
a
colon dash colon
a

Is what you’re proposing really about a fixed size of e.g., 5, or is it about the minimum possible cell size?

@vweevers
Copy link
Author

The paddedTable option in remark is about |a and b| (when false) and | a and b | (when true). It does this for the alignment row as well. But that’s different than having a fixed length.

I thought that is what spacedTable does. Given test.md:

|aaaa|bbbb|
|-|-|
|aaa|bbb|

{ spacedTable: true, paddedTable: false } yields:

| aaaa | bbbb |
| ---- | ---- |
| aaa | bbb |

The spaces are as expected (due to spacedTable), but I consider those added dashes in the alignment row to be padding.

Is what you’re proposing really about a fixed size of e.g., 5, or is it about the minimum possible cell size?

Either would work for me, but I guess minimum possible cell size is more sensible. I don't care what the size is, as long as it's stable. In other words, the behavior I'm looking for is that changing the content of a table cell on one line does not affect markdown on other lines (like alignment rows).

@wooorm
Copy link
Member

wooorm commented Aug 31, 2019

Whoops, I’m confused! 😅 Gah, you‘re right. That’s spaced. Padded is aligning the size of all cells.

I think this issue is about the alignment row, which currently, when not padding (as in, aligning), matches that of the first row (the header row).

How about this: remark-lint-table-pipe-alignment should have an option to support “aligned” (current behavior), “compact” (use the least space as possible). (And false of course, to not check it).

Then markdown-table should be updated to not depend on the header row for the alignment row, but instead use the least space as possible, when “pad: false”. this is a breaking change. We can make the names less confusing if we’re making breaking changes.

remark-stringify can then also have better names, and publish the breaking table change.

@vweevers
Copy link
Author

vweevers commented Aug 31, 2019

Sounds good!

There's a secondary issue: the 'compact' option of remark-lint-table-cell-padding (and remark-lint-table-pipe-alignment if it were to follow this behavior) matches { spacedTable: false, paddedTable: false }.

How about also adding a 'single' option, matching { spacedTable: true, paddedTable: false }? I.e.: exactly one space as padding margin.

wooorm added a commit to wooorm/markdown-table that referenced this issue Jan 23, 2020
The previous name was very confusing.

Related to remarkjs/remark-lint#217.
wooorm added a commit to wooorm/markdown-table that referenced this issue Jan 23, 2020
* Improve performance
* Fix a couple of bugs (such as trailing whitespace)
* Remove minimum of three character in alignment row cells
  — this isn’t needed in Markdown anymore
* Don’t double pad empty cells
* Fix tests

Related to remarkjs/remark-lint#217.
@wooorm
Copy link
Member

wooorm commented Mar 25, 2020

Sooo I’m reading this thread again wondering what I can do to solve it, but it’s really confusing because the terms are all weird: they’re getting better soon in remark though.

Both two rules currently do not check the alignment row. That could be a useful feature, but it would add more warnings. Especially what you’re asking at the start:

For this to be correct:

| foo | a long column | bar |
| --- | --- | --- |

And this to be incorrect:

|foo | a long column | bar|
|--- | ------- | -|

remark-lint-table-pipe-alignment checks whether the pipes align to form a pretty grid. They don’t in either example. And I no longer think a different option (“compact”) makes sense either: there’s either aligned, or not aligned.

remark-lint-table-cell-padding has two modes: padded, meaning there’s a space around pipes, which deems the first example as correct and the second as not, or compact, which deems both incorrect.

To conform to your ok/nok example:

  • remark-lint-table-pipe-alignment should not be used
  • remark-lint-table-cell-padding should be configured as padded, and it should check the alignment row

But due to changes in GitHub-style tables, that were applied to markdown-table, and will soon be in remark, your “correct” example should probably be:

| foo | a long column | bar |
| - | - | - |

The values in the aligning cells aren’t checked currently, it’s not really padding, so it only makes sense in a new rule, such as: remark-lint-table-compact-alignment-row (?), which would make sure that values are either -, :-:, :-, or -:: the smallest possible.

As this output isn’t currently possible with remark (but soon), I’d say that rule should be “tabled” for now.


For tables to be “nicely” aligned in a grid, such as:

| foo | short column      | bar |
| --- | ----------------- | --- |
| baz | a very long value | qux |

The two existing rules (remark-lint-table-pipe-alignment and remark-lint-table-cell-padding in padded) should check the alignment row as well.

wooorm added a commit to remarkjs/remark that referenced this issue Mar 28, 2020
This change updates markdown-table, the library used to serialize markdown
tables.

With that update comes options that make more sense:

* `looseTable` is no longer supported (it’s a bad GFM feature)
* `spacedTable` is now called `tableCellPadding`
* `paddedTable` is now called `tablePipeAlign`

Closes GH-476.
Related-to remarkjs/remark-lint#217.

Reviewed-by: Junyoung Choi <fluke8259@gmail.com>
Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com>
@wooorm
Copy link
Member

wooorm commented Jan 19, 2024

a) I’ve now added support to all the rules for checking the alignment rule
b) I rewrote remark-lint-table-cell-padding to be much smarter, it now knows of a minimum (when unaligned) and maximum (when aligned), I think we’d be basically there if we add padded-aligned and padded-unaligned to choose one number instead of a range
c) what remains is that that rule is about spaces, and this issue is about the “filler” dashes. With the new rules checking the alignment row, it now suggest to add spaces when aligning. So for example from:

| asd | qwe |
| - | -: |
| zxc | rty |

To:

| asd | qwe |
| -   |  -: |
| zxc | rty |

But I don’t see that often. I guess people like using a ton of - to fill that space. But there’s no real reason to do it. ...So...., still unsure where exactly this should go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have
Development

No branches or pull requests

2 participants