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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 18, 2018 •
edited by bedevere-bot

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
@serhiy-storchaka
bpo-34963: Make the repr of the typing.NewType() result more meaningful.
944d1d1 
Add attributes __qualname__ and __module__.
If the name argument is dotted, the __name__ attribute will be set to
its last component.
@gvanrossum gvanrossum removed their request for review Oct 18, 2018
ilevkivskyi
ilevkivskyi reviewed Oct 21, 2018
View changes
Copy link
Contributor

@ilevkivskyi ilevkivskyi left a comment

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

vstinner
vstinner requested changes Oct 24, 2018
View changes
Lib/typing.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Oct 24, 2018

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

vstinner
vstinner approved these changes Oct 28, 2018
View changes
Copy link
Member

@vstinner vstinner left a comment

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

@ilevkivskyi
Copy link
Contributor

@ilevkivskyi ilevkivskyi commented Oct 28, 2018

@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
Member Author

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

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

@ghost
Copy link

@ghost ghost commented Jan 10, 2019

Any news about PR?

@vstinner
Copy link
Member

@vstinner vstinner commented Jan 10, 2019

@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
Contributor

@ilevkivskyi ilevkivskyi commented Jan 11, 2019

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
Contributor

@gaborbernat gaborbernat commented Jun 5, 2020

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

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented May 2, 2021

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

gvanrossum
gvanrossum approved these changes Jun 22, 2021
View changes
Copy link
Member

@gvanrossum gvanrossum left a comment

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

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jun 22, 2021

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
Member

@gvanrossum gvanrossum commented Jun 22, 2021

@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
Member Author

@serhiy-storchaka serhiy-storchaka commented Jun 22, 2021

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

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jun 22, 2021

@serhiy-storchaka
Copy link
Member Author

@serhiy-storchaka serhiy-storchaka commented Jul 24, 2021 •
edited by bedevere-bot

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 Jul 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilevkivskyi ilevkivskyi

@vstinner vstinner

@gvanrossum gvanrossum

Assignees
No one assigned
Labels
awaiting merge CLA signed type-enhancement
Projects
None yet
Milestone
No milestone
8 participants
@serhiy-storchaka @bedevere-bot @ilevkivskyi @vstinner @gaborbernat @JelleZijlstra @gvanrossum @the-knights-who-say-ni