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 some tests based on coverage report: Part 2 #375

Merged
merged 6 commits into from
Feb 10, 2017

Conversation

ilevkivskyi
Copy link
Member

This is a continuation of #373

Here, however, a (very small) change to typing.py was necessary. The two PRs boost coverage to virtually 100% (if one does not count passes in abstract methods and other trivial things).

def __eq__(self, other):
if not isinstance(other, _TypeAlias):
return NotImplemented
return self.name == other.name and self.type_var == other.type_var
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't this check impl_type and type_checker?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an internal class, we know that if name is the same, then impl_type and type_checker must also be the same.

@ilevkivskyi
Copy link
Member Author

OK, It looks like the simplified version depends on changes in #383.
I will leave it like this until #383 is (hopefully) merged.

@gvanrossum Do you mind if we make GenericMeta code a bit more complex for 4x memory and 2x speed optimization? (this came up on python-dev some time ago)

@gvanrossum
Copy link
Member

I don't mind in principle, but I still haven't found the energy to review your code. I hope you can find someone else on python-dev to help you review your types changes if Inada doesn't feel comfortable.

@ilevkivskyi ilevkivskyi merged commit 370a137 into python:master Feb 10, 2017
@ilevkivskyi ilevkivskyi deleted the fixes-from-cov branch February 10, 2017 19:34
@ilevkivskyi
Copy link
Member Author

@gvanrossum
I have merged discussed PRs, but there is one thing that needs to be fixed before merging this into CPython: I added a test module mod.py some time ago (it was required to verify that cross-module type caching works right, a bug was reported). I will fix import for this module, and then it will be necessary to copy it to /Lib/test (where ann_module.py etc live). I will let you know, when everything is ready (probably later today).

@gvanrossum
Copy link
Member

gvanrossum commented Feb 10, 2017 via email

@gvanrossum
Copy link
Member

One more thing, "mod.py" is a rather non-descript name. Can you come up with something better?

@ilevkivskyi
Copy link
Member Author

Can you come up with something better?

Yes, exactly, this is among the things I am going to do today.

@ilevkivskyi
Copy link
Member Author

@gvanrossum I fixed the remaining issue. As I understand the current workflow, a PR should me made to master, and after it is merged, the corresponding squash-commit should be cherry-picked to other branches (3.5 and 3.6 in this case).
I have made a PR to CPython master python/cpython#28

By the way git can in principle cherry-pick between repos, but I tried this and it feels too complicated, because typing and cpython have quite different directory structure (it is quite easy however to cherry-pick a single commit between repos).

@gvanrossum
Copy link
Member

Thanks! Godspeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants