Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-34963: Make the repr of the typing.NewType() result more meaningful.#9951

Closed
serhiy-storchaka wants to merge 4 commits into
python:mainpython/cpython:mainfrom
serhiy-storchaka:typing-newtype-qualnameserhiy-storchaka/cpython:typing-newtype-qualnameCopy head branch name to clipboard
Closed

bpo-34963: Make the repr of the typing.NewType() result more meaningful.#9951
serhiy-storchaka wants to merge 4 commits into
python:mainpython/cpython:mainfrom
serhiy-storchaka:typing-newtype-qualnameserhiy-storchaka/cpython:typing-newtype-qualnameCopy head branch name to clipboard

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Oct 18, 2018

Copy link
Copy Markdown
Member

Add attributes __qualname__ and __module__.
If the name argument is dotted, the __name__ attribute will be set to its last component.

https://bugs.python.org/issue34963

Add attributes __qualname__ and __module__.
If the name argument is dotted, the __name__ attribute will be set to
its last component.
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Oct 18, 2018
@gvanrossum gvanrossum removed their request for review October 18, 2018 23:11

@ilevkivskyi ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I actually like this solution. But before merging let us wait for the benchmarks for other approach, if they are OK, maybe we can have an even nicer repr in #9808

Comment thread Lib/typing.py Outdated
@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but I would prefer that one of the typing maintainer double check this change. @ilevkivskyi?

@ilevkivskyi

Copy link
Copy Markdown
Member

@vstinner The PR looks good. However, there is a "competing" PR (alternative solution) in #9808. I asked the author for benchmarks, but he didn't respond yet. If he will not respond next week, I would propose to merge this one.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

I worked also on alternate implementations. But I don't know what properties should the NewType result have.

@ghost

ghost commented Jan 10, 2019

Copy link
Copy Markdown

Any news about PR?

@vstinner

Copy link
Copy Markdown
Member

@serhiy-storchaka: This PR still LGTM. You can merge it.

However, there is a "competing" PR (alternative solution) in #9808.

It doesn't seem to define module. It doesn't seem to be directly related. This PR looks simple enough.

@ilevkivskyi

Copy link
Copy Markdown
Member

It looks like the author of other PR is not responding. I leave up to Serhiy whether to just move with this PR or incorporate some ideas from other one too as I proposed in https://bugs.python.org/issue34963#msg328707

(This PR LGTM as is.)

@gaborbernat

Copy link
Copy Markdown
Contributor

@serhiy-storchaka shall we move ahead with this or the alternate PR?

@JelleZijlstra

Copy link
Copy Markdown
Member

This PR still applies and it would be useful, is there anything blocking it from being merged?

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, looks fine to me. I'll merge.

@gvanrossum

Copy link
Copy Markdown
Member

Closing and reopening since it looks like some tests are hanging.

@gvanrossum gvanrossum closed this Jun 22, 2021
@gvanrossum gvanrossum reopened this Jun 22, 2021
@gvanrossum

Copy link
Copy Markdown
Member

@serhiy-storchaka Can you look at the test failure? Maybe it just needs a merge from a more recent main branch?

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

It needs some correction in is_new_type() in Objects/unionobject.c.

@gvanrossum

gvanrossum commented Jun 22, 2021 via email

Copy link
Copy Markdown
Member

@serhiy-storchaka

serhiy-storchaka commented Jul 24, 2021

Copy link
Copy Markdown
Member Author

typing.NewType has been reimplemented as a class in bpo-44353, so this trick is no longer needed.

@serhiy-storchaka serhiy-storchaka deleted the typing-newtype-qualname branch July 24, 2021 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting merge type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.