Apply blackdoc to python source

[akaszynski]

PyVista has adopted black, but we fail to implement this for our documentation or doc strings.

This PR implements blackdoc to reformat all Python code in our docstrings as a pre-commit.

Pinging @jorgepiloto because this is really cool and IMO works better than blacken-docs

[tkoyama010]

Excellent. I was always concerned that the docstring code was not formatted by black. This solves that!

[codecov]

Codecov Report

Merging #4119 (84b326f) into main (a313b23) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4119   +/-   ##
=======================================
  Coverage   95.56%   95.56%           
=======================================
  Files          95       95           
  Lines       20247    20247           
=======================================
  Hits        19350    19350           
  Misses        897      897           

[adeak]

Any chance that we can separate the black run from any other change (i.e. adding this to pre-commit) into separate PRs? I'd like to be able to skip the style PR's merge commit in the git blame. As usual.

[adeak]

To be clear the ideal situation would be to separate all autoformatting. If you changed anything manually and know what that was, that should also be moved along with the other changes (because we want to see all human actions in the blame). With one big commit that's probably not feasible though.

And along with the pre-commit addition we should add a check for this in CI, otherwise noncomforming commits will startvseeping back quick.

[tkoyama010]

And along with the pre-commit addition we should add a check for this in CI, otherwise noncomforming commits will startvseeping back quick.

I guess the pre-commit error will stop CI, isn't it?

pyvista/.github/workflows/style_docstr.yml

Lines 16 to 26 in 5ff97bb

	- name: Set up Python
	uses: actions/setup-python@v4
	with:
	python-version: 3.9
	cache: 'pip'
	- name: Install pre-commit
	run: pip install pre-commit
	- name: Run pre-commit
	run: pre-commit run --all-files

[adeak]

Aaah, right! Thanks @tkoyama010 👍

[MatthewFlamm]

This would be a good example where a merge commit would be better to preserve the commit history in a PR (one commit for each step here), but we have disabled this (for good reasons).

Another thought is that you could have two PRs, one for adding to our pre-commit (this will fail), the other for the changes. You would merge the first if the second passes and then merge them in one after the other.

A lot of extra work however.

[adeak]

Another thought is that you could have two PRs, one for adding to our pre-commit (this will fail), the other for the changes. You would merge the first if the second passes and then merge them in one after the other.

That's what I had in mind. First merge the autoformatting changes, then adding the pre-commit hook and any hypothetical manual changes needed.

A lot of extra work however.

Only if @akaszynski touched formatting beyond running blackdoc. The non-autoformat changes would be quite obvious otherwise. And we can only do this right once, now, or we'll be stuck with crap history.

It's of course possible that I'm the only one looking at git blames, in which case "sucks to be me" is another option :P

[akaszynski]

Agree with separating this out. I didn't consider git blame, but when I do need it it's quite valuable for tracking down bugs.

[adeak]

You could keep most of the changes here, and just revert the few other changes. Whichever is less of a hassle.

[akaszynski]

Ok @adeak. I went ahead and opened #4120. I would like to merge the changes from branch into main with and also add git blame --ignore-rev.

After the merge I will add the commit hash to the . git-blame-ignore-revs file.

See
https://akrabat.com/ignoring-revisions-with-git-blame/
https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/

[akaszynski]

Please note that the changes here are entirely from the auto-formatter. Recommending merging this PR first and adding the ignore rev file to #4120.

[adeak]

Please note that the changes here are entirely from the auto-formatter. Recommending merging this PR first and adding the ignore rev file to #4120.

I'm on mobile for the next few hours still, but if this has autoformatting only then this is good to go. Any manual corrections would have to be put elsewhere anyway.

Adding the ignore-revs item in the follow-up is fine too. For the original blackening we had #2684.

Thanks a lot @akaszynski.

[MatthewFlamm]

This sounds like a good plan

[adeak]

Also pointing out for posterity that the changes here were made with an explicit exclusion of pyvista/plotting/charts.py, see #4120 (comment).

[akaszynski]

I’ll work on the follow up PR and implement the changes before enforcing blackdox. Thanks for the comments, I’ll be less trigger happy…

[adeak]

I’ll be less trigger happy…

It's all good, I wouldn't have approved if I had insisted on taking a close look before merge :) In the worst case of changing the config and rerunning blackdoc on the whole codebase we can just have another autoformat PR and add two commits to the ignore-revs file. No harm done.