Codecov Report

Merging #170 (7ec218c) into main (9425304) will increase coverage by 0.01%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
+ Coverage   97.22%   97.23%   +0.01%     
==========================================
  Files           8        8              
  Lines         648      651       +3     
  Branches       90       90              
==========================================
+ Hits          630      633       +3     
  Partials       18       18              

@tkoyama010 @banesullivan @akaszynski I think there are some Python 3.6 and Azure Pipeline tests failing here that are due for sunset pretty soon. Do you see any items I am missing in this PR that I should fix?

I'm surprised we're still supporting Python 3.6. Can we drop this @pyvista/developers?

@akaszynski I noticed some of the conda tests are failing because they don't have mypy, but it's in the requirements_tests.txt file for pip. Should I add that dependency to the environment.yml?

@akaszynski @adamgranthendry Absolutely. Please merge #164 .

@akaszynski @tkoyama010 Are either of you against me adding pre-commit like in pyvista as part of this PR as well?

Are either of you against me adding pre-commit like in pyvista as part of this PR as well?

@adam-grant-hendry +1 Let's add it!

Are either of you against me adding pre-commit like in pyvista as part of this PR as well?

@adam-grant-hendry +1 Let's add it!

I would prefer adding this in a separate PR as it's harder to tell which changes are simply a refactor and which changes are genuine feature changes.

@akaszynski @tkoyama010 One note: python 3.7 only supports numpy up to version 1.21.6.

I would prefer adding this in a separate PR as it's harder to tell which changes are simply a refactor and which changes are genuine feature changes.

Shoot, I saw this after I made the changes...any chance I could convince you to reconsider?

@akaszynski @tkoyama010 I'm not sure why coverage has dropped. I think it has something to do with line length. I started by making all lengths <= 72 and then increased it to 100, but rather than increase coverage it decreased it. I'm not sure what's going on. Can you help?

Otherwise, all other tests are passing.

I would prefer adding this in a separate PR as it's harder to tell which changes are simply a refactor and which changes are genuine feature changes.

Shoot, I saw this after I made the changes...any chance I could convince you to reconsider?

I'm just the peanut gallery here, because I don't developy pyvistaqt. But for pyvista, having blackening commits separately (i.e. having blackening PRs separately) means that I can use a git-ignore-revs-file that lets me see a blame not tainted by code churn caused by black. See pyvista/pyvista#2684. Depending on whether current and any future pyvistaqt devs think this is useful, this would be a strong motivator to pull out the style changes into a separate PR.

(If you want to do this it's probably easier to turn this branch into a blackening branch, and creating a new branch for your #163 fixing changes.)

I would prefer adding this in a separate PR as it's harder to tell which changes are simply a refactor and which changes are genuine feature changes.

Shoot, I saw this after I made the changes...any chance I could convince you to reconsider?

I'm just the peanut gallery here, because I don't developy pyvistaqt. But for pyvista, having blackening commits separately (i.e. having blackening PRs separately) means that I can use a git-ignore-revs-file that lets me see a blame not tainted by code churn caused by black. See pyvista/pyvista#2684. Depending on whether current and any future pyvistaqt devs think this is useful, this would be a strong motivator to pull out the style changes into a separate PR.

(If you want to do this it's probably easier to turn this branch into a blackening branch, and creating a new branch for your #163 fixing changes.)

Are you recommending I separate concerns then and create a separate branch?

Well, like I said your choice here isn't likely to affect me now or in the future, so I'm hesitant to pressure you in any way :) Just an additional (significant) data point for your choice.

But yes, what I'm saying is that if you separate the trivial style change churn into a separate PR, then it can be ignored in git blames (both locally and on github, as of recently). This is all necessary because we squash on merge, so a complete PR becomes a single commit.

Well, like I said your choice here isn't likely to affect me now or in the future, so I'm hesitant to pressure you in any way :) Just an additional (significant) data point for your choice.

But yes, what I'm saying is that if you separate the trivial style change churn into a separate PR, then it can be ignored in git blames (both locally and on github, as of recently). This is all necessary because we squash on merge, so a complete PR becomes a single commit.

Your views are valued and suggestions valued. I leave the decision up to @akaszynski and @tkoyama010 . Well have to revert some merges, but I'm ok with a group decision. We're the ones who will have to maintain things.

@adeak and loathe though I am to redo work, I agree with you that it makes sense to separate concerns. I believe @akaszynski shares the same sentiment.

For what it's worth "we" often mix and match things in PRs, and while it's not as pure as I'd like I don't think there's anything inherently wrong with that as long as the result is reviewable, and if critical changes are not buried in an unrelated PR.

This here is a technicality that our commits are in one-to-one correspondence with PRs. If this weren't the case we could leave this as is, and list a dozen commits from this PR in the ignore-revs file.