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

bpo-40389: Improve repr of typing.Optional #19714

Merged
merged 5 commits into from Apr 30, 2020
Merged

Conversation

Endilll
Copy link
Contributor

@Endilll Endilll commented Apr 25, 2020

@the-knights-who-say-ni
Copy link

@the-knights-who-say-ni the-knights-who-say-ni commented Apr 25, 2020

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@Endilll

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Lib/typing.py Outdated
@@ -683,6 +683,9 @@ def copy_with(self, params):
return _GenericAlias(self.__origin__, params, name=self._name, inst=self._inst)

def __repr__(self):
if (self.__origin__ == Union and len(self.__args__) == 2
and self.__args__[1] is type(None)):
Copy link
Contributor

@remilapeyre remilapeyre Apr 25, 2020

Choose a reason for hiding this comment

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

Hi @Endilll, thanks for opening a pull request for this. Notice that this does not work, when None is the first element:

>>> import typing
>>> typing.Union[None, int]
typing.Union[NoneType, int]
>>> typing.Union[None, int] == typing.Optional[int]
True

This change will also need some tests and an NEWS entry.

Copy link
Contributor Author

@Endilll Endilll Apr 25, 2020

Choose a reason for hiding this comment

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

Thank you for the feedback. Fixed.

@remilapeyre
Copy link
Contributor

@remilapeyre remilapeyre commented Apr 25, 2020

You can add a NEWS entry using https://blurb-it.herokuapp.com/

@aeros
Copy link
Member

@aeros aeros commented Apr 26, 2020

Thanks for the PR and welcome @Endilll.

FYI: assuming the idea is approved by the maintainers of typing, this change will most definitely require some updates to the tests in test_typing.py (in Lib/test).

@Endilll Endilll requested a review from ericvsmith as a code owner Apr 26, 2020
Copy link
Contributor

@ilevkivskyi ilevkivskyi left a comment

Looks good, thanks! Also be sure to sign the CLA.

The Azure failure look like a flake, but I can't re-trigger it, and it is required for merging. @zooba do you know how to fix this?

Copy link
Member

@gvanrossum gvanrossum left a comment

LGTM. Please sign the CLA so we can merge.

@Endilll
Copy link
Contributor Author

@Endilll Endilll commented Apr 27, 2020

I've submitted CLA back on Saturday, and bot says that one business day is needed to update the records, so I suppose we just need to wait a bit.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Apr 30, 2020

Closing and reopening to re-run the Azure pipelines.

@gvanrossum gvanrossum closed this Apr 30, 2020
@gvanrossum gvanrossum reopened this Apr 30, 2020
@gvanrossum gvanrossum merged commit 138a9b9 into python:master Apr 30, 2020
14 checks passed
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 30, 2020

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Apr 30, 2020

Congratulations!

@Endilll
Copy link
Contributor Author

@Endilll Endilll commented Apr 30, 2020

I want to thank everybody for such a welcoming attitude.

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.

None yet

7 participants