NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

MNT: pre-commit rst linter + small linting fixes#26737

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

timhoffm merged 4 commits intomatplotlib:mainfromstory645:rst-linter

Sep 15, 2023

Merged

MNT: pre-commit rst linter + small linting fixes#26737

timhoffm merged 4 commits intomatplotlib:mainfromstory645:rst-linter

Sep 15, 2023

Conversation

Copy link

Member

story645 commentedSep 11, 2023•
edited
Loading

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

story645 added the Maintenance label

Sep 11, 2023

story645 marked this pull request as draft

September 11, 2023 22:16

story645 force-pushed therst-linter branch 2 times, most recently frome057fd4 tod51c3a8Compare

September 12, 2023 05:10

story645 changed the title~~MNT: pre-commit rst linter~~MNT: pre-commit rst linter + small linting fixes

Sep 12, 2023

story645 marked this pull request as ready for review

September 12, 2023 05:12

oscargus reviewed

Sep 12, 2023

View reviewed changes

doc/users/next_whats_new/updated_mathtext.rst OutdatedShow resolvedHide resolved

oscargus reviewed

Sep 12, 2023

View reviewed changes

galleries/users_explain/toolkits/axisartist.rst Outdated

		- baseline
		- center
		- baseline

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is no improvement IMHO...

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It fixes the linter error?

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If the code works, is the linter correct in complaining?

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

timhoffmSep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

As long as the tables are small (like in this example), I prefer ReSTsimple 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.

Copy link

MemberAuthor

story645Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

timhoffmSep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

story645Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

timhoffmSep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Copy link

MemberAuthor

story645Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

oscargus commentedSep 12, 2023

I love static checking tools, so in general I am very positive to this. However, the table code looks quite unmanageable...

Copy link

MemberAuthor

story645 commentedSep 12, 2023

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.

Copy link

MemberAuthor

story645 commentedSep 12, 2023

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.

ksunden reviewed

Sep 12, 2023

View reviewed changes

doc/devel/MEP/MEP28.rst OutdatedShow resolvedHide resolved

story645 mentioned this pull request

Sep 12, 2023

DOC: separate and clarify axisartist default tables#26754

Merged

1 task

Copy link

MemberAuthor

story645 commentedSep 12, 2023

@oscargus I opened#26754 for axisartist. Still has list table mostly because I think that's more robust across different editing environments.

story645 force-pushed therst-linter branch from45945d1 to78c898bCompare

September 12, 2023 19:14

QuLogic reviewed

Sep 12, 2023

View reviewed changes

doc/devel/contribute.rst OutdatedShow resolvedHide resolved

doc/devel/document.rst OutdatedShow resolvedHide resolved

pyproject.toml OutdatedShow resolvedHide resolved

story645 force-pushed therst-linter branch from78c898b toc49c316Compare

September 13, 2023 02:16

QuLogic reviewed

Sep 13, 2023

View reviewed changes

doc/_templates/autosummary.rstShow resolvedHide resolved

doc/api/prev_api_changes/api_changes_2.1.2.rst OutdatedShow resolvedHide resolved

doc/devel/MEP/MEP23.rst OutdatedShow resolvedHide resolved

pyproject.toml OutdatedShow resolvedHide resolved

story645 force-pushed therst-linter branch fromc49c316 to016c2a7Compare

September 13, 2023 04:16

rstcheck config and precommit & fixed rst linting errors

41d1963

story645 force-pushed therst-linter branch from016c2a7 to41d1963Compare

September 13, 2023 06:57

story645 added2 commits

September 14, 2023 22:27

use inline to ignore role in file instead of ignoring error

55dee12

axis artist param table in simple table format

e6805e6

story645 force-pushed therst-linter branch from92e2024 toe6805e6Compare

September 15, 2023 02:40

ghpull directive

4f7d68f

timhoffm approved these changes

Sep 15, 2023

View reviewed changes

timhoffm added this to thev3.8-doc milestone

Sep 15, 2023

timhoffm merged commit5e441da intomatplotlib:main

Sep 15, 2023

Copy link

lumberbot-appbot commentedSep 15, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v3.8.xgit pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 5e441da55ba1be8eb06c9f26b21d195d10971173

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #26737: MNT: pre-commit rst linter + small linting fixes'

Push to a named branch:

git push YOURFORK v3.8.x:auto-backport-of-pr-26737-on-v3.8.x

Create a PR against branch v3.8.x, I would have named this PR:

"Backport PR#26737 on branch v3.8.x (MNT: pre-commit rst linter + small linting fixes)"

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 theStill Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free tosuggest an improvement.

lumberbot-appbot added the Still Needs Manual Backport label

Sep 15, 2023

Copy link

lumberbot-appbot commentedSep 15, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v3.8.0-docgit pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 5e441da55ba1be8eb06c9f26b21d195d10971173

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #26737: MNT: pre-commit rst linter + small linting fixes'

Push to a named branch:

git push YOURFORK v3.8.0-doc:auto-backport-of-pr-26737-on-v3.8.0-doc

Create a PR against branch v3.8.0-doc, I would have named this PR:

"Backport PR#26737 on branch v3.8.0-doc (MNT: pre-commit rst linter + small linting fixes)"

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 theStill Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free tosuggest an improvement.

timhoffm added the Documentation label

Sep 15, 2023

story645 deleted the rst-linter branch

September 15, 2023 07:02

Copy link

Member

timhoffm commentedSep 15, 2023

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 removed the Still Needs Manual Backport label

Sep 15, 2023

Copy link

MemberAuthor

story645 commentedSep 15, 2023

@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).

Copy link

Member

ksunden commentedSep 15, 2023

The backport failed because it changedwhats-new/api doc entries (mostly addingpython tocode-block directives) that were deleted as part of release preparation, I'll add back in the changes to the resulting files in the merge-up process.

story645 added a commit to story645/matplotlib that referenced this pull request

Sep 15, 2023

Backport changes to contribute from PRmatplotlib#26737

00591a0

story645 added a commit to story645/matplotlib that referenced this pull request

Sep 15, 2023

Backport changes to contribute from PRmatplotlib#26737

8c50ae8

rcomer mentioned this pull request

Sep 18, 2023

Backport changes to contribute from PR #26737#26789

Merged

rcomer added a commit that referenced this pull request

Sep 18, 2023

Merge pull request#26789from story645/auto-backport-of-pr-26737-on-…

406a5ea

…v3.8.xBackport changes to contribute from PR#26737

rcomer mentioned this pull request

Sep 18, 2023

Backport changes to contribute from PR #26737#26790

Merged

rcomer added a commit that referenced this pull request

Sep 18, 2023

Merge pull request#26790from story645/auto-backport-of-pr-26737-on-…

bf0b600

…v3.8.0-docBackport changes to contribute from PR#26737

ksunden mentioned this pull request

Nov 2, 2023

merge up v3.8.1#27256

Merged

5 tasks

Labels

Documentation Maintenance

5 participants

Movatterモバイル変換

Uh oh!

MNT: pre-commit rst linter + small linting fixes#26737

MNT: pre-commit rst linter + small linting fixes#26737

Uh oh!

Conversation

story645 commentedSep 11, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscargus commentedSep 12, 2023

Uh oh!

story645 commentedSep 12, 2023

Uh oh!

story645 commentedSep 12, 2023

Uh oh!

Uh oh!

story645 commentedSep 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lumberbot-appbot commentedSep 15, 2023

Uh oh!

lumberbot-appbot commentedSep 15, 2023

Uh oh!

timhoffm commentedSep 15, 2023

Uh oh!

story645 commentedSep 15, 2023

Uh oh!

ksunden commentedSep 15, 2023

Uh oh!

Uh oh!

story645 commentedSep 11, 2023•
edited
Loading