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

Adding type annotations to several files #760

Merged
merged 9 commits into from May 7, 2018

Conversation

Projects
None yet
4 participants
@kylepjohnson
Copy link
Member

kylepjohnson commented May 6, 2018

Looking for @todd-cook 's thoughts about my type annotations to cltk/utils/philology.py (and to less extent cltk/utils/contributors.py).

also cc @jtauber

I ran mypy with mypy --ignore-missing-imports cltk/utils/philology.py. (Also messed with stuff to get a near-perfect pylint score.) Tests passing on my updated branch.

It took me some hours to grok the annotations, however now that I kinda get them -- I am totally sold on their value. But if I am doing something crazy, don't hesitate to let me know.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 6, 2018

Codecov Report

Merging #760 into master will decrease coverage by <.01%.
The diff coverage is 92.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
- Coverage   87.76%   87.76%   -0.01%     
==========================================
  Files         148      148              
  Lines        8828     8834       +6     
==========================================
+ Hits         7748     7753       +5     
- Misses       1080     1081       +1
Impacted Files Coverage Δ
cltk/vector/word2vec.py 18.51% <ø> (ø) ⬆️
cltk/utils/contributors.py 98.55% <100%> (+0.13%) ⬆️
cltk/tokenize/latin_exceptions.py 100% <100%> (ø) ⬆️
cltk/utils/philology.py 89.62% <88.5%> (-0.77%) ⬇️
cltk/tests/test_utils.py 96.19% <90.9%> (-0.11%) ⬇️

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 36ba994...1c8c3bf. Read the comment docs.

@todd-cook
Copy link
Contributor

todd-cook left a comment

All of these changes look excellent.
I think the more we adopt annotations, the more we can help guarantee the library is called appropriately and behaves as promised.

@todd-cook

This comment has been minimized.

Copy link
Contributor

todd-cook commented May 6, 2018

A few notes about MyPy:
If your code calls other libraries, you will see more warnings about missing type information, so it may be difficult to get the MyPy output to be rather silent. Still, most active libraries are adding annotations, and this support will only get better.

Besides the obvious standard types, Any and NamedTuple are helpful for annotating user defined types and data coming from external libraries (e.g. SqlAlchemy, and others).

The true value of MyPy comes during active development, when it uncovers bugs before finding them at runtime. I'm glad you're becoming a fan!

@kylepjohnson

This comment has been minimized.

Copy link
Member Author

kylepjohnson commented May 7, 2018

Thanks Todd!

If your code calls other libraries, you will see more warnings about missing type information, so it may be difficult to get the MyPy output to be rather silent.

I skimmed the mypy doc pages on stubs and typeshed. I kinda get it. My instinct is the get down the type annotations for our own library, then move on to stubs (or whatever) to make up the missing remainder.

Besides the obvious standard types, Any and NamedTuple are helpful for annotating user defined types and data coming from external libraries (e.g. SqlAlchemy, and others).

OK, I'll read about this.

@kylepjohnson kylepjohnson merged commit 667ba6f into cltk:master May 7, 2018

3 checks passed

codecov/patch 92.36% of diff hit (target 87.76%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +4.59% compared to 36ba994
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kylepjohnson kylepjohnson deleted the kylepjohnson:learn-mypy branch May 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.