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

Regression table-cell-padding from 9.1.2 to 10.0.0 when evaluating header alignment #317

Closed
4 tasks done
fmigneault opened this issue Apr 10, 2024 · 11 comments
Closed
4 tasks done
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on

Comments

@fmigneault
Copy link

fmigneault commented Apr 10, 2024

Initial checklist

Affected packages and versions

remark-lint-table-cell-padding

Link to runnable example

No response

Steps to reproduce

Run make check-md using https://github.com/crim-ca/weaver.
This should trigger an error on this table: https://github.com/crim-ca/weaver/blob/73fe9a5a1c56047c73648f30e084961777ab806a/SECURITY.md?plain=1#L11-L14

Alternatively, install the packages yourself (https://github.com/crim-ca/weaver/blob/73fe9a5a1c56047c73648f30e084961777ab806a/Makefile#L287-L299), and run the remark-lint command (https://github.com/crim-ca/weaver/blob/73fe9a5a1c56047c73648f30e084961777ab806a/Makefile#L616-L623).

Expected behavior

A table using no spaces between - used to separate the header was permitted.
The table-cell-padding rule was actually applied to cells, not the header alignment definition.

For example, the following did not raise any error (as expected).

| Version | Supported          |
|---------|--------------------|
| 4.x     | :white_check_mark: |
| < 4.x   | :x:                |

While the following did raise it:

| Version| Supported          |
|--------|--------------------|
| 4.x    | :white_check_mark: |
| < 4.x  | :x:                |

While I can understand wanting to favor | --- | instead of |---| to align with examples such as https://www.markdownguide.org/extended-syntax/#tables, there is no more any way to support the desired format. Setting padded forces adding extra spaces, consistent raises that they are not, and compact forces removing the spaces in the cells, making it less readable. There should be an option to allow controlling the header alignment padding separately from cells.

The motivation of this is that some IDEs, notably JetBrains, support auto-formating tables. When it does, it uses the formatting method of the first example, which is valid Markdown. With version 10.0.0, one as to completely disable table-cell-padding due to this limitation, which can lead to the introduction of miss-padded cells.

Actual behavior

The table

| Version | Supported          |
|---------|--------------------|
| 4.x     | :white_check_mark: |
| < 4.x   | :x:                |

Leads to (using padded):

12:2  warning Unexpected `0` spaces between cell edge and content, expected `1` space, add `1` space table-cell-padding remark-lint
12:11 warning Unexpected `0` spaces between cell content and edge, expected `1` space, add `1` space table-cell-padding remark-lint
12:12 warning Unexpected `0` spaces between cell edge and content, expected `1` space, add `1` space table-cell-padding remark-lint
12:32 warning Unexpected `0` spaces between cell content and edge, expected `1` space, add `1` space table-cell-padding remark-lint

Leads to (using compact):

11:3  warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
11:13 warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
13:3  warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
13:13 warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
14:3  warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
14:13 warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint

Leads to (using consistent):

11:3  warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
  [cause]:
    Cell padding style `'compact'` first defined for `'consistent'` here
11:13 warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
  [cause]:
    Cell padding style `'compact'` first defined for `'consistent'` here
13:3  warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
  [cause]:
    Cell padding style `'compact'` first defined for `'consistent'` here
13:13 warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
  [cause]:
    Cell padding style `'compact'` first defined for `'consistent'` here
14:3  warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
  [cause]:
    Cell padding style `'compact'` first defined for `'consistent'` here
14:13 warning Unexpected `1` space between cell edge and content, expected `0` spaces, remove `1` space table-cell-padding remark-lint
  [cause]:
    Cell padding style `'compact'` first defined for `'consistent'` here

Runtime

Node v16

Package manager

npm 8

OS

Linux

Build and bundle tools

Other (please specify in steps to reproduce)

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Apr 10, 2024
@fmigneault
Copy link
Author

@fmigneault
Copy link
Author

Corresponding issue created in https://youtrack.jetbrains.com/issue/PY-71886

@fmigneault
Copy link
Author

Relates to #217

@wooorm
Copy link
Member

wooorm commented Apr 12, 2024

Hey Francis! I don’t really understand, I think this is not a regression, but a feature.

there is no more any way to support the desired format.

There should be an option to allow controlling the header alignment padding separately from cells.

When it does, it uses the formatting method of the first example, which is valid Markdown.

It seems your issue is more with JetBrains formatting tables in a different way: personally I haven’t seen that style often.
You can’t expect remark-lint to be able to follow every style of every formatter ever?

If you use a formatter, then you don’t need this rule, right? So turning it off doesn’t matter?

@fmigneault
Copy link
Author

In a way, the |---| lines are not "cells", but the header and alignment rule, so I don't think the feature is doing exactly the thing it advertises. I think it is fairly common to have the |---| notation without applying the same padding/compact rule to all the cells.
It is even more common when table-pipe-alignment is not applied. For example, #217 referred to the following table that applies it this way: https://github.com/standard/standard/blob/3b3c316a358ad8d306229a59a67c92891e0827f3/README.md?plain=1#L183-L184

And as shown by the lines https://github.com/standard/standard/blob/3b3c316a358ad8d306229a59a67c92891e0827f3/README.md?plain=1#L177-L181, the leading | are missing.
So overall, when this limitation becomes a noisy nuisance, users simply turn it off instead, leading to actual linting errors and poorer quality.

@wooorm
Copy link
Member

wooorm commented Apr 13, 2024

In a way, the |---| lines are not "cells"

This is a stretch to me. They’re very much cells to me. They work like cells. Just, they contain dashes and colons. So I do think the feature is doing exactly what advertised.

It is even more common when table-pipe-alignment is not applied

Why do you think that whether cells are aligned, relates to whether cells are padded, in that way?

standard/standard@3b3c316/README.md

I see an inconsistent table there: sometimes padded, sometimes not. Importantly, the OP 217, mentions what should and should not be valid. And that such a rule would be useful to standard.

the leading | are missing.

This is terrible. A really bad style. If this proves anything, it’s that that table is not to be looked at for inspiration of any kind.

So overall, when this limitation becomes a noisy nuisance

Huh? I don’t understand how the sentence before can lead you to conclude this?

users simply turn it off instead, leading to actual linting errors and poorer quality.

If you want your own kind of tables, indeed, turn it off. You can make your own rules to match your own styles.
I think the things in the alignment row can definitely be considered cells, and so apply to this rule, and so will be either padded or unpadded. I don’t see a good reason to not pad things in the the alignment row if you do pad all other rows.

@fmigneault
Copy link
Author

This is a stretch to me. They’re very much cells to me. They work like cells.

If they were, they would be rendered as cells, but they are not. They are the line between the header and the other cells, plus have special meaning about the following cell's content alignment. They are very different from any other "normal" cell.

Why do you think that whether cells are aligned, relates to whether cells are padded, in that way?

When cells are not aligned, adding the extra padding is not that important, since it is not expected that they would line up. So the minimal |---| definition just to say "this is the header" is sufficient.

This is terrible. A really bad style. If this proves anything, it’s that that table is not to be looked at for inspiration of any kind.

Huh? I don’t understand how the sentence before can lead you to conclude this?

My point about this was that, due to the limitation of table-cell-padding configuration, the rule ended up being disabled entirely because it was causing too much noise, and then this kind of errors made their way into the code without every being flagged. So yes, they are terrible tables, but this highlights the need for more control over its configuration so that users apply an appropriate fix rather than ignore them entirely. When users start to fight against linters, they tend to ignore the issues if there is not a better setting to fix it.

If you want your own kind of tables, indeed, turn it off. You can make your own rules to match your own styles.

It don't think it is only a matter of "my own style". There are plenty of examples out there that use the proposed format.
For example, the first 3 matches obtained from Google "Markdown table generator" use exactly that format :

@wooorm
Copy link
Member

wooorm commented Apr 15, 2024

They are very different from any other "normal" cell.

I really very strongly disagree here.

When cells are not aligned, adding the extra padding is not that important, since it is not expected that they would line up. So the minimal |---| definition just to say "this is the header" is sufficient.

The minimal “filler” is -, not ---. So, when padded and with pipes: | - |. Or when not padded |-|.
And when aligned, it has to do with the longest cell in the column, when padded: | alpha |\n| ----- | or when not padded: |alpha|\n|-----|.

too much noise

How is this rule causing that?
Either you use a formatter (jetbrains?) and you don’t need this rule.
Or you can make your table consistent: padding every “cell-like thing” or not?
Or if you want things differently: feel free to turn it off and optionally make your own rule?

When users start to fight against linters, they tend to ignore the issues if there is not a better setting to fix it.

Users are always fighting against linters. Any new lint is annoying at first. That’s my experience with other linters. And then after a bit I get it.

codebeautify.org/markdown-table-generator

This one doesn’t show for me what you say it does.
But the rest indeed. Wild!

You’re the first person who disagrees with this style in 13 years, since I started with markdown. Our tools formatting markdown are more popular: https://github.com/wooorm/markdown-table is just the tables part. Which remark uses internally.

@fmigneault
Copy link
Author

Users are always fighting against linters. Any new lint is annoying at first. That’s my experience with other linters. And then after a bit I get it.

This is why I marked this issue as a "regression". It was not doing this before, but now it does. It is not a new thing "happening at first", it is something that was allowed for a very long time, and suddenly started triggered on every repo that use this format. And this, causes a lot of noise.

This one doesn’t show for me what you say it does.

It uses the |-| instead of |---|, but the "header/delimiter rule" is compact while the other cells are "padded".
But, I must admit, it is somewhat inconsistent because its title cells are compact as well, while cells below are not... So not the best example. Either way, it does not change the fact that using a "shortcut notation" for the header/delimiter rule without caring too much about how the actual cells are padded is sufficiently common to have a few examples online.

You’re the first person who disagrees with this style in 13 years

Since it was silently allowed before, it is not surprising if nobody raised concerns about it...

I'm not against or for any particular style, but rather, in the camp of "consistent across the repository". As long as the rule is applied the same way everywhere, I think this format variant is just as valid as the fully-padded or fully-compact one, particularly since we can find many examples online using it.

Regrettably, it seems the only option I have now is to disable the rule entirely, which then can lead to such inconsistencies. I would much rather have the option to explicitly check the format than ignoring it.

@wooorm
Copy link
Member

wooorm commented Apr 16, 2024

It wasn’t supported before and now it is. It’s a feature.

but the "header/delimiter rule" is compact while the other cells are "padded".

No, I do not see what you describe. I see padded cells:

Screenshot 2024-04-16 at 2 50 50 PM

it is not surprising if nobody raised concerns about it...

I describe that I am talking about formatting which is more popular than linting.

consistent across the repository

Then pad all your cells.

it seems the only option I have now is to disable the rule entirely

No, you can also pad your cells.


Feel free to make your own rules

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Apr 16, 2024
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Apr 16, 2024

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Apr 16, 2024
fmigneault-crim pushed a commit to crim-ca/weaver that referenced this issue Apr 20, 2024
… + combine npm configs into a single location
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

2 participants