remarkjs / remark-lint

plugins to check (lint) markdown code style
https://remark.js.org
MIT License
938 stars 129 forks source link

Support fixed-width table alignment row #217

Open vweevers opened 5 years ago

vweevers commented 5 years ago

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|
|--- | ------- | -|
wooorm commented 5 years ago

đŸ€” 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 commented 5 years ago

and warns about the invalid example

I should have given a more specific invalid example:

| foo | a long column | bar |
| --- | ------------- | --- |
vweevers commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

...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?

wooorm commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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:

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 commented 8 months ago

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!