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-41810: Reintroduce types.EllipsisType, .NoneType & .NotImplementedType #22336

Merged
merged 13 commits into from
Sep 22, 2020

Conversation

BvB93
Copy link
Contributor

@BvB93 BvB93 commented Sep 21, 2020

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

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:

@BvB93

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!

@BvB93 BvB93 changed the title BPO-41810: Reintroduce types.EllipsisType bpo-41810: Reintroduce types.EllipsisType Sep 21, 2020
@BvB93
Copy link
Contributor Author

BvB93 commented Sep 21, 2020

For reference: I just signed the CLA a few minutes ago.

Bas van Beek added 3 commits September 21, 2020 20:32
`type(NotImplemented)` and `type(None)` have, respectively, been replaced with `types.NotImplementedType` and `types.NoneType`
@BvB93 BvB93 changed the title bpo-41810: Reintroduce types.EllipsisType bpo-41810: Reintroduce types.EllipsisType, .NoneType & .NotImplementedType Sep 21, 2020
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

I've not done a thorough review yet. These comments are just about the blurb.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@BvB93
Copy link
Contributor Author

BvB93 commented Sep 21, 2020

The PR would be easier to review if you hadn’t tried to fix all occurrences of type(......). That could then be done in one or several follow up PRs. Or possibly just skipped until there is demand.

If you want I can remove those changes; they're localized to 0a97a88 and 2791a23 so reverting them should be straightforward.

@gvanrossum
Copy link
Member

If you want I can remove those changes; they're localized to 0a97a88 and 2791a23 so reverting them should be straightforward.

@ericvsmith, what do you think?

@ericvsmith
Copy link
Member

I do think it would be easier to review if they were moved to a separate PR, even if it's on the same bpo.

@BvB93
Copy link
Contributor Author

BvB93 commented Sep 21, 2020

I do think it would be easier to review if they were moved to a separate PR, even if it's on the same bpo.

The changes have been reverted as of 84ebe18.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I think the docs for the three special objects should not start with “The sole value of the type ...”.

The first sentence should serve as a brief summary of what the object is used for. You can then follow up with the detail about its singleton type.

Otherwise LGTM. Thanks for your patience!

Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@ericvsmith
Copy link
Member

I think the docs for the three special objects should not start with “The sole value of the type ...”.

It's just mirroring the existing documentation for None. But I agree that moving the "the sole value" sentence (or sentence fragment) to the end of the description would be an improvement.

@terryjreedy
Copy link
Member

I did not particularly want the change in IDLE, because 1) I am not sure I prefer it (partly because of the extra import) and I don't know that PEP-8 'mandates' it*, and 2) I cannot backport it to actively maintained 3.8 and 3.9 code, so it would cause a merge conflict if I patched that section of code. I checked that the latter seems unlikely in this particular case.

I think the effort to 'sanitize' all existing code is futile, since people will continue to put type(None) in code. Consider leading existing code alone.

Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

Approved, but only with misleading wording fixed.

@@ -19,14 +19,15 @@ A small number of constants live in the built-in namespace. They are:

.. data:: None

The sole value of the type ``NoneType``. ``None`` is frequently used to
The sole value of the type :data:`types.NoneType`. ``None`` is frequently used to
Copy link
Member

Choose a reason for hiding this comment

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

"The sole value of the type NoneType" is most easily read (parsed) as referring to NoneType's usefulness. I expected it to be followed by 'is to ...' and initially read 'is frequently' as a grammatical error. It took awhile to realize that this is a convoluted way to say 'None'. I strongly recommend a rewording.

Suggested change
The sole value of the type :data:`types.NoneType`. ``None`` is frequently used to
''None'' is the only instance of :data:`types.NoneType`. It is frequently used to

types.NoneType is already twice identified as a type, so a third mention is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on your and Guido's feedback (#22336 (review)) I'm proposing something like this:

.. data:: None

   An object frequently used to represent the absence of a value, as when
   default arguments are not passed to a function. Assignments to ``None``
   are illegal and raise a :exc:`SyntaxError`.
   Sole instance of the :data:`types.NoneType` type.

Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

That's much better, I think. Although I'd make the last sentence (with the correct markup): "None is the sole instance of the NoneType type."

Comment on lines 29 to 30
Sole value of the type :data:`types.NotImplementedType` and a
special value which should be returned by the binary special methods
Copy link
Member

Choose a reason for hiding this comment

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

See above. Use a parallel construction.

Suggested change
Sole value of the type :data:`types.NotImplementedType` and a
special value which should be returned by the binary special methods
``NotImplemented is the only instance of :data:`types.NotImplementedType`. It is a
special value which should be returned by the binary special methods

@@ -59,7 +60,8 @@ A small number of constants live in the built-in namespace. They are:
.. index:: single: ...; ellipsis literal
.. data:: Ellipsis

The same as the ellipsis literal "``...``". Special value used mostly in conjunction
The same as the ellipsis literal "``...``" and sole value of the type
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The same as the ellipsis literal "``...``" and sole value of the type
The same as the ellipsis literal "``...``" and the only instance of


Reintroduced the :data:`types.EllipsisType`, :data:`types.NoneType`
and :data:`types.NotImplementedType` classes, providing a new set
of types readily interpretable by type checkers.
Copy link
Member

Choose a reason for hiding this comment

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

I think this and the blurb are fine, providing a reason without saying either that this is the only reason or that these should be use everywhere.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@BvB93
Copy link
Contributor Author

BvB93 commented Sep 22, 2020

@terryjreedy the documentation changes have been implemented as of 23ed559.

@gvanrossum gvanrossum merged commit 0d0e9fe into python:master Sep 22, 2020
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Sep 24, 2020
* origin/master: (27 commits)
  bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388)
  bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387)
  bpo-41833: threading.Thread now uses the target name (pythonGH-22357)
  bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633)
  bpo-33822: Update IDLE section of What's New 3.8 (pythonGH-22383)
  bpo-41844: Add IDLE section to What's New 3.9 (GN-22382)
  bpo-41841: Prepare IDLE News for 3.10 (pythonGH-22379)
  bpo-37779 : Add information about the overriding behavior of ConfigParser.read (pythonGH-15177)
  bpo-40170: Use inline _PyType_HasFeature() function (pythonGH-22375)
  bpo-40941: Fix stackdepth compiler warnings (pythonGH-22377)
  bpo-40941: Fix fold_tuple_on_constants() compiler warnings (pythonGH-22378)
  bpo-40521: Fix PyUnicode_InternInPlace() (pythonGH-22376)
  bpo-41834: Remove _Py_CheckRecursionLimit variable (pythonGH-22359)
  bpo-1635741, unicodedata: add ucd_type parameter to UCD_Check() macro (pythonGH-22328)
  bpo-1635741: Port _lsprof extension to multi-phase init (PEP 489) (pythonGH-22220)
  bpo-41513: Improve order of adding fractional values. Improve variable names. (pythonGH-22368)
  bpo-41816: `StrEnum.__str__` is `str.__str__` (pythonGH-22362)
  bpo-35764: Rewrite the IDLE Calltips doc section  (pythonGH-22363)
  bpo-41810: Reintroduce `types.EllipsisType`, `.NoneType` & `.NotImplementedType` (pythonGH-22336)
  bpo-41602: raise SIGINT exit code on KeyboardInterrupt from pymain_run_module (python#21956)
  ...
@BvB93 BvB93 deleted the ellipsis branch September 28, 2020 16:51
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants