GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add some tests based on coverage report: Part 2 by ilevkivskyi · Pull Request #375 · python/typing

Conversation

@ilevkivskyi

@ilevkivskyi ilevkivskyi commented Jan 28, 2017

Copy link
Copy Markdown
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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
JelleZijlstra
JelleZijlstra reviewed Jan 28, 2017
View reviewed changes
Comment thread python2/typing.py
def __eq__(self, other):
if not isinstance(other, _TypeAlias):
return NotImplemented
return self.name == other.name and self.type_var == other.type_var

@JelleZijlstra JelleZijlstra Jan 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ilevkivskyi ilevkivskyi Jan 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ilevkivskyi ilevkivskyi mentioned this pull request Jan 31, 2017
Sometimes bot does not add "CLA signed" label python/the-knights-who-say-ni#87
Closed
@brettcannon brettcannon added CLA not signed and removed CLA not signed labels Jan 31, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 31, 2017
@ilevkivskyi ilevkivskyi mentioned this pull request Feb 3, 2017
Prepend bases on first subscription #386
Merged
ilevkivskyi added 3 commits February 3, 2017 23:41
@ilevkivskyi
Add more tests based on coverage report
46c47ab
@ilevkivskyi
Fix lint on PY2
ddf5078
@ilevkivskyi
Simplify PR to only tests and tiny code clean-up
43a1cb6
@ilevkivskyi
ilevkivskyi force-pushed the fixes-from-cov branch from 88a658b to 43a1cb6 Compare February 3, 2017 22:42
@ilevkivskyi
Remove _right_ code on PY2
cf4e6a6
@ilevkivskyi

ilevkivskyi commented Feb 3, 2017

Copy link
Copy Markdown
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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Feb 3, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ivan Levkivskyi and others added 2 commits February 10, 2017 20:27
Ivan Levkivskyi
Re-run Travis
7156cf7
@ilevkivskyi
Merge branch 'master' into fixes-from-cov
5f69d62
@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

ilevkivskyi commented Feb 10, 2017

Copy link
Copy Markdown
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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Feb 10, 2017 via email

Copy link
Copy Markdown
Member
OK. Since the move to git is currently happening I can't merge anything into CPython anyway right now, so there's no particular hurry. I wonder if git can cherry-pick across repos, that would be amazing. Otherwise I guess it will have to be a series of PRs to the new cpython repo (see https://paper.dropbox.com/doc/CPython-workflow-changes-mx1k8G6M0rg5JLy80F1r6 ).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Feb 10, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ilevkivskyi

ilevkivskyi commented Feb 10, 2017

Copy link
Copy Markdown
Member Author

Can you come up with something better?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ilevkivskyi

ilevkivskyi commented Feb 11, 2017

Copy link
Copy Markdown
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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Feb 12, 2017

Copy link
Copy Markdown
Member

Thanks! Godspeed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@ilevkivskyi @gvanrossum @JelleZijlstra @brettcannon @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.