MNT: pre-commit rst linter + small linting fixes #26737
Conversation
e057fd4 to
d51c3a8
Compare
| - baseline | ||
| - center | ||
| - baseline | ||
|
|
There was a problem hiding this comment.
This is no improvement IMHO...
There was a problem hiding this comment.
It fixes the linter error?
There was a problem hiding this comment.
If the code works, is the linter correct in complaining?
There was a problem hiding this comment.
The original table was formatted incorrectly - the second line was supposed to be = not -
I didn't copy and paste that fix to the symlinked file and that's probably why the linter was still cranky but also I personally find list-table and grid table orders of magnitude easier to maintain than the tables where I have to update the whole thing on every change in space.
There was a problem hiding this comment.
As long as the tables are small (like in this example), I prefer ReST simple tables. Yes they are more cumbersome to update but they are much easier to read, and readability counts. It's not that we are changing these tables all the time.
There was a problem hiding this comment.
I'm struggling a bit with your comment. I think there are two issues with this table:
-
I had trouble understanding what the table was trying to communicate, so I reworked the table in its own PR.
-
simple table vs list table format - I always prefer the latter because I find it more portable across different development environments. I understand the readibility concern but I can pull up the rendered docs to see the table, and also the list-table format still clearly delineates rows and columns and that's the important part for editing.
Because of 1, I'd rather discussions of this table move to that PR. If the table format is a blocker then I'll change it there.
There was a problem hiding this comment.
Because of 1, I'd rather discussions of this table move to that PR. If the table format is a blocker then I'll change it there.
I'm fine with that approach. I'm just wondering why you bothered to rewrite the old table here, when all it needs for a technical fix is replace the - by =, and for content changes, you again have the separate PR.
There was a problem hiding this comment.
Honestly cause I made that change and the linter didn't pass & then I made this change & remembered to C&P it to the non-updating symlinked file and it did pass and since I like this format better anyway I didn't feel like going back and making a way I didn't even like work.
There was a problem hiding this comment.
tried to put it in simple table to close out this pr and that also actually requires a content change - it's also not happy with the ? table titles
|
I love static checking tools, so in general I am very positive to this. However, the table code looks quite unmanageable... |
|
I find the list tables way easier to work with then the simple tables since I don't have to worry about spacing? It's long yes, but each row is distinct. |
|
Granted, I also do not understand that table in the least - what are the first two columns? - but fixing that is way out of scope for this PR. |
45945d1 to
78c898b
Compare
78c898b to
c49c316
Compare
c49c316 to
016c2a7
Compare
016c2a7 to
41d1963
Compare
92e2024 to
e6805e6
Compare
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
|
Do we actually need this in the 3.8-doc branch? While it conceptually should go there, it should be sufficient that the test runs on the master branch to catch any rst issues of new contributions. So I'd claim we don't have to bother with a manual backport. But feel free to decide otherwise. |
|
@timhoffm yeah my take is that this is primarily a dev tool & I can manually split off the few docs I'd want in earlier (primarily the contribute page changes as I've got the separate PR for the axisartist page). |
|
The backport failed because it changed |
…v3.8.x Backport changes to contribute from PR #26737
…v3.8.0-doc Backport changes to contribute from PR #26737
Adds rstcheck as a pre-commit hook, mostly cause I seem to have a mental block about underlines being same length as title. This pre-commit also has the advantage of being pretty configurable.
The .rst files in this PR are the linting errors I could fix because I wanted to minimize the ignored errors as much as possible.