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

Feature/type annotations #904

Merged
merged 28 commits into from
Mar 5, 2020
Merged

Feature/type annotations #904

merged 28 commits into from
Mar 5, 2020

Conversation

midhun-pm
Copy link
Contributor

@midhun-pm midhun-pm commented Feb 17, 2020

PR adds

  • Mypy Type annotations for the exposed traits api (in progress)

  • A (minimal) testing framework for the type annotations

  • Tests for the annotated types

@corranwebster
Copy link
Contributor

I have a fairly strong recommendation to re-think the testing code. Rather than trying to extract code from within a function to run Mypy on, I'd go the much simpler route of having a tests.examples submodule containing the sample code you want to run Mypy on, and have the tests just grab a local .py file by path and feed it into Mypy directly.

@corranwebster
Copy link
Contributor

This installs correctly for me and is working as expected now (within the limits of what has been implemented).

@codecov-io
Copy link

codecov-io commented Feb 17, 2020

Codecov Report

Merging #904 into master will increase coverage by 0.79%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
+ Coverage    72.2%   72.99%   +0.79%     
==========================================
  Files          51       51              
  Lines        6367     6474     +107     
  Branches     1277     1302      +25     
==========================================
+ Hits         4597     4726     +129     
+ Misses       1379     1349      -30     
- Partials      391      399       +8
Impacted Files Coverage Δ
traits/editor_factories.py 81.25% <0%> (-2.97%) ⬇️
traits/trait_list_object.py 75.09% <0%> (ø) ⬆️
traits/testing/unittest_tools.py 96.03% <0%> (ø) ⬆️
traits/trait_errors.py 83.33% <0%> (ø) ⬆️
traits/trait_handlers.py 61.02% <0%> (ø) ⬆️
traits/trait_numeric.py 51.65% <0%> (ø) ⬆️
traits/interface_checker.py 87.87% <0%> (ø) ⬆️
traits/api.py 90.32% <0%> (ø) ⬆️
traits/trait_notifiers.py 79.11% <0%> (ø) ⬆️
traits/constants.py 100% <0%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 653b169...aacf612. Read the comment docs.

@corranwebster
Copy link
Contributor

Playing around with this, we probably do need to have basic __init__ method signatures for the TraitTypes if the signature differs from the base TraitType signature. For example I am getting errors of the form "Enum has too many arguments" for constructs like Enum('foo', 'bar', 'baz').

We can use the @overload decorator if there are multiple different valid signatures (as is the case for Enum)

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

Before this is merged, please could you add a README text file at the top level of the traits-stubs directory that explains briefly what the code in this directory is doing and how to use it? We'll ultimately need more documentation than that, but a README would be enough for now.

traits-stubs/setup.py Outdated Show resolved Hide resolved
@mdickinson
Copy link
Member

A couple of other comments:

  • the package is called traits-stubs, which is not a valid Python package name; maybe rename the package to traits_stubs (but keep the containing directory name of traits-stubs)? I'm not sure how / where this might matter, though.

  • is the CI running the tests at the moment? If not, please could you open an issue for running the tests under the CI? (Or even better, fix in this PR, if the fix is easy.)

@corranwebster
Copy link
Contributor

  • the package is called traits-stubs, which is not a valid Python package name; maybe rename the package to traits_stubs (but keep the containing directory name of traits-stubs)? I'm not sure how / where this might matter, though.

The naming of the package is important, see PEP 0561

@corranwebster
Copy link
Contributor

Yes, tests need to be run by CI and run successfully as part of this PR.

@mdickinson
Copy link
Member

The naming of the package is important, see PEP 0561

Thanks. So to have a proper package structure for the tests, we may need to move the tests outside the traits-stubs directory. It may be worth checking how other stubs-providing packages do their testing.

etstool.py Outdated Show resolved Hide resolved
etstool.py Outdated Show resolved Hide resolved
etstool.py Outdated Show resolved Hide resolved
traits-stubs/README.rst Outdated Show resolved Hide resolved
@mdickinson
Copy link
Member

N.B. For the Python 3.5 CI failures, it's probably enough to just replace open(some_path) with some_path.open(). While you're doing that, please also supply the encoding to the open call.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Merging.

@mdickinson mdickinson merged commit 8d539c9 into master Mar 5, 2020
@mdickinson mdickinson deleted the feature/type_annotations branch March 5, 2020 07:15
@midhun-pm midhun-pm mentioned this pull request Mar 12, 2020
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.

None yet

4 participants