Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💚 Add mypy in CI #47

Merged
merged 65 commits into from
Sep 30, 2020
Merged

💚 Add mypy in CI #47

merged 65 commits into from
Sep 30, 2020

Conversation

tkoyama010
Copy link
Member

Let's add mypy in CI. We need type hint for this.
mypy logo

@codecov
Copy link

Codecov bot commented Jul 31, 2020

Codecov Report

Merging #47 into master will decrease coverage by 0.10%.
The diff coverage is 98.82%.

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   98.06%   97.95%   -0.11%     
==========================================
  Files           6        6              
  Lines         516      539      +23     
==========================================
+ Hits          506      528      +22     
- Misses         10       11       +1     

@GuillaumeFavelier
Copy link
Contributor

GuillaumeFavelier commented Aug 3, 2020

As described in mypy documentation, by default mypy will assume the functions to be dynamically typed and only check the imported modules. I went ahead and ignored those imports in favor of --disallow-untyped-defs. I think it suits better your intention @tkoyama010 ?

@tkoyama010
Copy link
Member Author

Thanks for @GuillaumeFavelier 's work.

@tkoyama010 tkoyama010 marked this pull request as ready for review September 26, 2020 01:35
@tkoyama010 tkoyama010 marked this pull request as draft September 26, 2020 01:42
@tkoyama010
Copy link
Member Author

tkoyama010 commented Sep 26, 2020

☝️ This error is the Type Annotation problem of Python3.5. I think we should finish support of Python3.5.
See #60 and pyvista/pyvista#905 .

@tkoyama010 tkoyama010 marked this pull request as ready for review September 26, 2020 13:11
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
@tkoyama010 tkoyama010 marked this pull request as draft September 27, 2020 01:02
@tkoyama010 tkoyama010 marked this pull request as ready for review September 28, 2020 23:22
@tkoyama010
Copy link
Member Author

tkoyama010 commented Sep 29, 2020

This is ready for reviews @akaszynski @GuillaumeFavelier @banesullivan @larsoner

@akaszynski
Copy link
Member

I'm all for static type checking, especially since it's optional. While this won't replace unit testing, it can help us identify errors (and help the IDE). I'd like to hear the opinions of the other contributors as well.

Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

I haven't always been a fan of type annotations but lately, they have been growing on me. Thanks for taking this on to move pyvistaqt in a good direction, @tkoyama010!

This works well here because it is a smaller codebase, but I think it would be tough to do in pyvista core

@akaszynski
Copy link
Member

This works well here because it is a smaller codebase, but I think it would be tough to do in pyvista core

Agreed. We can just do it in chunks since mypy is optional.

@akaszynski akaszynski merged commit 24373c3 into pyvista:master Sep 30, 2020
@tkoyama010 tkoyama010 deleted the patch-5 branch October 1, 2020 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants