Skip to content

WIP: more annotations #3090

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

Merged
merged 4 commits into from
Jul 11, 2019
Merged

WIP: more annotations #3090

merged 4 commits into from
Jul 11, 2019

Conversation

crusaderky
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 9, 2019

Codecov Report

Merging #3090 into master will increase coverage by 0.44%.
The diff coverage is 70.58%.

@@            Coverage Diff            @@
##           master   #3090      +/-   ##
=========================================
+ Coverage   92.16%   92.6%   +0.44%     
=========================================
  Files          63      63              
  Lines       12787   12799      +12     
  Branches     2906    2907       +1     
=========================================
+ Hits        11785   11853      +68     
+ Misses        688     630      -58     
- Partials      314     316       +2

@crusaderky crusaderky closed this Jul 9, 2019
@crusaderky crusaderky reopened this Jul 9, 2019
self,
chunks: Union[
None,
Number,
Copy link
Member

Choose a reason for hiding this comment

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

Last I checked, type checkers were much happier with int rather than numbers.Number. Maybe mypy has been updated since then, though....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dask will happily accept float, numpy.int64, etc. and transparently cast them to int.
Should we decide that dask is excessively nice and the intended input must strictly be int?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I vote to lean towards wider input bounds - it much more annoying for users to have false negatives than false positives; so in this case Number assuming mypy understands that

Copy link
Member

Choose a reason for hiding this comment

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

OK, let's give this a try. We can always change later, that's not a big deal.

One other minor note -- it would be great to define a few type aliases for these longer types. The line wrapping you've done here helps, but this is still very long to write in a function signature.

@shoyer shoyer merged commit 8f0d9e5 into pydata:master Jul 11, 2019
@shoyer
Copy link
Member

shoyer commented Jul 11, 2019

Thanks @crusaderky. This is great, please keep them coming!

By the way, codecov updates its coverage reports as the tests on Azure complete. So usually in the first comment you'll see absurdly low coverage, but if you wait another ten minutes it will edit with the complete results. I don't know how to fix this easily, but fortunately it's only a minor annoyance.

@crusaderky
Copy link
Contributor Author

@shoyer On the first run the pytests themselves (not codecov) were showing half of the tests as "skipped". As if all optional dependencies randomly and silently failed to install. Not sure why. As a direct result codecov was showing a drop in coverage.

@crusaderky crusaderky deleted the annotations branch July 11, 2019 08:40
@max-sixty max-sixty mentioned this pull request Jul 11, 2019
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.

3 participants