'remark-lint-table-pipe-alignment' treats blank cells incorrectly

simnalamburt commented 6 months ago

Initial checklist

[X] I read the support docs
[X] I read the contributing guide
[X] I agree to follow the code of conduct
[X] I searched issues and couldn’t find anything (or linked relevant results below)

Affected packages and versions

main branch of 'remark-lint-table-pipe-alignment' package

Link to runnable example

https://github.com/simnalamburt/remark-lint/tree/issue-312

Steps to reproduce

Run node test.js at https://github.com/simnalamburt/remark-lint/tree/issue-312

I added following test cases: (https://github.com/remarkjs/remark-lint/compare/main...simnalamburt:issue-312)

diff --git a/packages/remark-lint-table-pipe-alignment/index.js b/packages/remark-lint-table-pipe-alignment/index.js
index 8a70fc9..caf4f47 100644
--- a/packages/remark-lint-table-pipe-alignment/index.js
+++ b/packages/remark-lint-table-pipe-alignment/index.js
@@ -67,6 +67,10 @@
  *   |------|---------------:|
  *   |Venus |         50 115 |
  *
+ *   | abc |
+ *   |:---:|
+ *   |     |
+ *
  * @example
  *   {"gfm": true, "label": "input", "name": "not-ok.md"}
  *

Expected behavior

It should be report OK

Actual behavior

It errors like below

✖ failing tests:

test at file:/Users/simnalamburt/workspace/remark-lint/test.js:317:13
✖ ok.md:{"config":true} (2.929458ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
  + actual - expected

  + [
  +   '13:7: Unexpected unaligned cell, expected aligned pipes, add `-2` spaces',
  +   '13:7: Unexpected unaligned cell, expected aligned pipes, add `2` spaces'
  + ]
  - []
      at assertCheck (file:///Users/simnalamburt/workspace/remark-lint/test.js:361:10)
      at async TestContext.<anonymous> (file:///Users/simnalamburt/workspace/remark-lint/test.js:318:7)
      at async Test.run (node:internal/test_runner/test:632:9)
      at async assertPlugin (file:///Users/simnalamburt/workspace/remark-lint/test.js:317:5)
      at async TestContext.<anonymous> (file:///Users/simnalamburt/workspace/remark-lint/test.js:294:7)
      at async Test.run (node:internal/test_runner/test:632:9)
      at async TestContext.<anonymous> (file:///Users/simnalamburt/workspace/remark-lint/test.js:293:5)
      at async Test.run (node:internal/test_runner/test:632:9)
      at async Test.processPendingSubtests (node:internal/test_runner/test:374:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: [Array],
    expected: [],
    operator: 'deepStrictEqual'
  }

Runtime

node.js v20.11.1

Package manager

npm v10.2.4

OS

macOS

Build and bundle tools

None

simnalamburt commented 6 months ago

I found that it also fails for table that look like this. Is this intended behavior or a bug?

<!-- the following fails -->
|    x|
|:---:|
|    x|

<!-- the following are okay -->
|x    |
|:---:|
|x    |

simnalamburt commented 6 months ago

Since info.size.left and info.size.right can be undefined in the following inputs, there seems to be no way to reliably get the left and right margins of a cell in the current implementation.

 abc | abc
:---:|:---:
x    |x
 x   | x
  x  |  x
   x |   x
    x|    x
     |

The important thing for this plugin is to get the position of the | correct, not so much whether the space is added to the left or right to get the position of the | correct. Therefore, I would like to propose that info.align with "center" be treated the same as "left" like https://github.com/simnalamburt/remark-lint/commit/7a2f9bef96c91fc770a745f24783702dc9676b4a. What do you think?

Reference

Patch: https://github.com/simnalamburt/remark-lint/commit/7a2f9bef96c91fc770a745f24783702dc9676b4a

simnalamburt commented 6 months ago

New issue: current implementation treats left padding incorrectly when the first | is omitted.

 abc |
-----|
 x   |

This is because current implementation of inferSize returns info.size.left as undefined. I would like to propose to fix the implementation of inferSize so that info.size.left always has a positive integer value. What do you think?

Reference

Bug reproduction: https://github.com/simnalamburt/remark-lint/commit/6661a4bbc7c650381b8f2524dfe9d52d5069df7c

simnalamburt commented 6 months ago

I noticed that the markdown ASTs that are initially fed into this plugin have the left padding information removed.

Input:

1.  yolo

       a |
    -----|
    aaaaa|

AST:

{
  type: 'root',
  children: [
    {
      type: 'list',
      ordered: true,
      start: 1,
      spread: false,
      children: [
        {
          type: 'listItem',
          spread: true,
          checked: null,
          children: [
            {
              type: 'paragraph',
              children: [
                {
                  type: 'text',
                  value: 'yolo',
                  position: {
                    start: { line: 1, column: 5, offset: 4 },
                    end: { line: 1, column: 9, offset: 8 }
                  }
                }
              ],
              position: {
                start: { line: 1, column: 5, offset: 4 },
                end: { line: 1, column: 9, offset: 8 }
              }
            },
            {
              type: 'table',
              align: [ null ],
              children: [
                {
                  type: 'tableRow',
                  children: [
                    {
                      type: 'tableCell',
                      children: [
                        {
                          type: 'text',
                          value: 'a',
                          position: {
                            start: { line: 3, column: 8, offset: 17 },
                            end: { line: 3, column: 9, offset: 18 }
                          }
                        }
                      ],
                      position: {
                        start: { line: 3, column: 8, offset: 17 },
                        end: { line: 3, column: 11, offset: 20 }
                      }
                    }
                  ],
                  position: {
                    start: { line: 3, column: 8, offset: 17 },
                    end: { line: 3, column: 11, offset: 20 }
                  }
                },
                {
                  type: 'tableRow',
                  children: [
                    {
                      type: 'tableCell',
                      children: [
                        {
                          type: 'text',
                          value: 'aaaaa',
                          position: {
                            start: { line: 5, column: 5, offset: 36 },
                            end: { line: 5, column: 10, offset: 41 }
                          }
                        }
                      ],
                      position: {
                        start: { line: 5, column: 5, offset: 36 },
                        end: { line: 5, column: 11, offset: 42 }
                      }
                    }
                  ],
                  position: {
                    start: { line: 5, column: 5, offset: 36 },
                    end: { line: 5, column: 11, offset: 42 }
                  }
                }
              ],
              position: {
                start: { line: 3, column: 8, offset: 17 },
                end: { line: 5, column: 11, offset: 42 }
              }
            }
          ],
          position: {
            start: { line: 1, column: 1, offset: 0 },
            end: { line: 5, column: 11, offset: 42 }
          }
        }
      ],
      position: {
        start: { line: 1, column: 1, offset: 0 },
        end: { line: 5, column: 11, offset: 42 }
      }
    }
  ],
  position: {
    start: { line: 1, column: 1, offset: 0 },
    end: { line: 5, column: 11, offset: 42 }
  }
}

There are two ways to resolve this issue.

Fix micromark-extension-gfm-table

Looking at the micromark-extension-gfm-table implementation, it seems to have been implemented without considering the case where there is left padding in the first place and the first | is omitted. This seems to be the reason why the left padding information is removed from the AST.
Workaround without fixing micromark-extension-gfm-table

Traverse the "tableRow" AST nodes and store the leftmost column position information and use it.

What do you think?

wooorm commented 5 months ago

Wow that’s a lot of text! Thanks for your ideas but this is really hard to get through.

micromark-extension-gfm-table is correct. The row/cell/text indeed all start at a. Not on some particular whitespace between.

github-actions[bot] commented 5 months ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

wooorm commented 5 months ago

I fixed the issue you reported, about empty centered cells. I also improved the second part, which you reported: weirdly aligned centered cells, where the pipes are actually aligned.

As for the “centered” cells of rows without initial |: it is unclear how they should behave. Markdown allows indenting things. That indent is different from the “padding” you use to indent cells. You also cannot “indent”/“pad” cells like you show:

 abc | abc
:---:|:---:
x    |x
 x   | x
  x  |  x
   x |   x
    x|    x
     |

Yields:

abc	abc
x	x
x	x
x	x
x	x
x	x

(note that the “end” of the table, is not a table, but becomes code)

simnalamburt commented 5 months ago

note that the “end” of the table, is not a table, but becomes code

Wow I actually didn't notice that part. Thanks for the clarification and quick fix!

remarkjs / remark-lint