bpo-34963: Make the repr of the typing.NewType() result more meaningful. by serhiy-storchaka · Pull Request #9951 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation18 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Add attributes qualname and module. If the name argument is dotted, the name attribute will be set to its last component.

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

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

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?

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

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

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

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.)

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

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.

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

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

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

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