Fixes Issue #163 by adam-grant-hendry · Pull Request #170 · pyvista/pyvistaqt

adam-grant-hendry · 2022-06-28T01:58:10Z

Issue #163 states failure to use redundant alias imports causes mypy to generate error messages about not being able to find parent methods.

The first commit in this PR adds a purposefully failing regression test, demonstrating the error indeed occurs. The second commit corrects the error and causes the test to pass.

This fixes Issue #163

…havior

codecov · 2022-06-28T02:01:26Z

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

PEP 484 requires redundant alias imports to identify modules as being re-exported. Adding these fixes the mypy error that it cannot find parent methods in child instances of `MainWindow`. Fixes Issue pyvista#163

adam-grant-hendry · 2022-06-28T02:24:49Z

@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?

akaszynski · 2022-06-28T02:26:16Z

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

adam-grant-hendry · 2022-06-28T02:32:27Z

@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?

tkoyama010 · 2022-06-28T02:33:05Z

@akaszynski @adamgranthendry Absolutely. Please merge #164 .

Conda tests need `mypy` for new regression test.

adam-grant-hendry · 2022-06-28T03:04:45Z

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

tkoyama010 · 2022-06-28T03:07:14Z

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!

akaszynski · 2022-06-28T03:10:30Z

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.

.pylintrc

…th `pyvista`

…ectly by pre-commit

…trc` The removed options were removed from `pylint` in PR4942 and PR 3571. See also: https://pylint.pycqa.org/en/latest/whatsnew/2/2.6/summary.html#summary-release-highlights

- `setup.cfg` was typed incorrectly and still specified python >= 3.6 instead of >= 3.7 - `check-jsonschema` was typed incorrectly in `requirements_test.txt` and python 3.7 only supports up to numpy 1.21.6

adam-grant-hendry · 2022-06-28T04:22:12Z

@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?

adam-grant-hendry · 2022-06-28T05:58:50Z

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

…line length

adeak · 2022-06-28T11:13:40Z

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

adam-grant-hendry · 2022-06-28T11:25:08Z

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?

adeak · 2022-06-28T11:28:25Z

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.

adam-grant-hendry · 2022-06-28T11:38:00Z

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.

adam-grant-hendry · 2022-06-28T11:39:49Z

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

adeak · 2022-06-28T11:42:22Z

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.

fix(typehint): add failing regression test to demonstrate existing be…

1f716b9

…havior

fix(typehint): use redundant alias imports to fix mypy errors

3166587

PEP 484 requires redundant alias imports to identify modules as being re-exported. Adding these fixes the mypy error that it cannot find parent methods in child instances of `MainWindow`. Fixes Issue pyvista#163

tkoyama010 and others added 2 commits June 28, 2022 11:34

Merge branch 'main' into fix/typehint

978764e

fix(mypy): add mypy to environment.yml dependencies

ac7911a

Conda tests need `mypy` for new regression test.

adam-grant-hendry mentioned this pull request Jun 28, 2022

Bump pylint from 2.13.0 to 2.14.3 #175

Closed

tkoyama010 reviewed Jun 28, 2022

View reviewed changes

.pylintrc Outdated Show resolved Hide resolved

adamgranthendry and others added 5 commits June 27, 2022 20:49

fix(formatting): add pre-commit and fix formatting to better align wi…

e91226f

…th `pyvista`

Merge branch 'main' into fix/typehint

a10703e

fix(requirements): remove check-json-schema as this is imported dir…

d0a42ce

…ectly by pre-commit

refactor(pylintrc): remove commented-out disable options from `.pylin…

e6f1b45

…trc` The removed options were removed from `pylint` in PR4942 and PR 3571. See also: https://pylint.pycqa.org/en/latest/whatsnew/2/2.6/summary.html#summary-release-highlights

fix(dependencies): correct python and pip requirements

3441552

- `setup.cfg` was typed incorrectly and still specified python >= 3.6 instead of >= 3.7 - `check-jsonschema` was typed incorrectly in `requirements_test.txt` and python 3.7 only supports up to numpy 1.21.6

adamgranthendry added 3 commits June 27, 2022 21:25

fix(classifiers): correct trove classifiers

6f5fb98

fix(pylint): correct pylint errors and add to pre-commit

bb5da9a

refactor(line-length): change max line length to 100

5dce1b0

adamgranthendry added 4 commits June 27, 2022 23:27

feat(test): add test for baseplotter noop and revert to 72 character …

92015f4

…line length

refactor(line-lengths): revert to 100-character line lengths

55ad30d

fix(baseplotter-noop): revert to using empty lambda for noop

2a82336

fix(baseplotter_noop): revert to one-arg noop lambda for base plotter

7ec218c

adeak linked an issue Jun 28, 2022 that may be closed by this pull request

Bug: mypy cannot access QMainWindow methods from MainWindow #163

Open

adam-grant-hendry requested a review from tkoyama010 June 28, 2022 15:29

adam-grant-hendry closed this Jun 28, 2022

adam-grant-hendry deleted the fix/typehint branch June 28, 2022 15:39

Conversation

adam-grant-hendry commented Jun 28, 2022

Uh oh!

codecov bot commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

akaszynski commented Jun 28, 2022

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

tkoyama010 commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

tkoyama010 commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akaszynski commented Jun 28, 2022

Uh oh!

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

adeak commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

adeak commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

adam-grant-hendry commented Jun 28, 2022

Uh oh!

adeak commented Jun 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jun 28, 2022 •

edited

Loading

tkoyama010 commented Jun 28, 2022 •

edited

Loading

tkoyama010 commented Jun 28, 2022 •

edited

Loading

adeak commented Jun 28, 2022 •

edited

Loading

adeak commented Jun 28, 2022 •

edited

Loading