GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-91896: Improve visibility of `ByteString` deprecation warnings by AlexWaygood · Pull Request #104294 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented May 8, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

#102096 added deprecation warnings for collections.abc.ByteString, but these warnings are currently only emitted on subclassing ByteString or calling isinstance() against it. Importing ByteString or accessing it via import collections.abc; collections.abc.ByteString does not currently trigger any warnings, meaning somebody who's just using the class for type annotations might not get any warning that the class will be removed in 3.14. Moreover, the isinstance() deprecation warnings are only emitted on isinstance() checks against collections.abc.ByteString, not on isinstance() checks against typing.ByteString. The docs update that we just made in 1f56795 says that typing.ByteString will also be removed in Python 3.14, so we should emit the same deprecation warnings for typing.ByteString.

This PR adds deprecation warnings for:

  • from collections.abc import ByteString
  • import collections.abc; collections.abc.ByteString
  • from typing import ByteString
  • import typing; typing.ByteString
  • isinstance(b"", typing.ByteString)
  • Issue: Deprecate and schedule removal of collections.abc.ByteString and typing.ByteString #91896

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AlexWaygood
pythongh-91896: Improve visibility of ByteString deprecation warnings
424486a
@AlexWaygood AlexWaygood added skip news topic-typing labels May 8, 2023
@AlexWaygood
AlexWaygood requested a review from hauntsaninja May 8, 2023 15:36
@AlexWaygood
AlexWaygood requested review from Fidget-Spinner, JelleZijlstra, gvanrossum and rhettinger as code owners May 8, 2023 15:36
@bedevere-bot bedevere-bot mentioned this pull request May 8, 2023
Deprecate and schedule removal of collections.abc.ByteString and typing.ByteString #91896
Closed
@bedevere-bot bedevere-bot added the awaiting core review label May 8, 2023
@AlexWaygood

AlexWaygood commented May 8, 2023

Copy link
Copy Markdown
Member Author

(Skipping news as I think the news entry in #102096 should be enough)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
AlexWaygood marked this pull request as draft May 8, 2023 15:57
@AlexWaygood
Fix test_regrtest (hopefully)
8015ba3
@AlexWaygood
AlexWaygood force-pushed the more-bytestring-warnings branch from 8c469cf to 8015ba3 Compare May 8, 2023 16:38
@AlexWaygood
AlexWaygood marked this pull request as ready for review May 8, 2023 16:55
Eclips4
Eclips4 reviewed May 8, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Multiple inheritance with typing.ByteString also emits a warning
67973d1
@rhettinger

rhettinger commented May 9, 2023

Copy link
Copy Markdown
Contributor

I'm not opposed to this but think it isn't necessary. We aren't required to emit warnings. The deprecation notices in whatsnew and the docs will suffice. This doubly true for ByteString which like has low use.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented May 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I'm not opposed to this but think it isn't necessary. We aren't required to emit warnings. The deprecation notices in whatsnew and the docs will suffice. This doubly true for ByteString which like has low use.

I don't feel strongly about this, and perhaps it isn't strictly necessary, but I would prefer maximum visibility, personally. I think it will help users if they get a clear runtime warning when they use the symbol in a type annotation, before the symbol is removed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
hauntsaninja
hauntsaninja reviewed May 9, 2023
View reviewed changes

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks! While collections.abc.ByteString is extremely low use, there is some use of typing.ByteString as an annotation out there, for which this could help.

Either way we should lean on type checkers for this deprecation as well, since we won't get warnings in stub files or some from __future__ import annotations scenarios.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
hauntsaninja
hauntsaninja approved these changes May 12, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 12, 2023
@hauntsaninja
hauntsaninja merged commit f0f5bb3 into python:main May 12, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label May 12, 2023
@AlexWaygood
AlexWaygood deleted the more-bytestring-warnings branch May 12, 2023 07:30
This was referenced May 12, 2023
gh-91896: Fixup some docs issues following ByteString deprecation #104422
Merged
gh-91896: Revert some very noisy DeprecationWarnings for ByteString #104424
Merged
carljm added a commit to carljm/cpython that referenced this pull request May 12, 2023
@carljm
Merge branch 'main' into supermon
8efd7c6 
* main:
  pythongh-91896: Fixup some docs issues following ByteString deprecation (python#104422)
  pythonGH-104371: check return value of calling `mv.release` (python#104417)
  pythongh-104415: Fix refleak tests for `typing.ByteString` deprecation (python#104416)
  pythonGH-86275: Implementation of hypothesis stubs for property-based tests, with zoneinfo tests (python#22863)
  pythonGH-103082: Filter LINE events in VM, to simplify tool implementation. (pythonGH-104387)
  pythongh-93649: Split gc- and allocation tests from _testcapimodule.c (pythonGH-104403)
  pythongh-104389: Add 'unused' keyword to Argument Clinic C converters (python#104390)
  pythongh-101819: Prepare _io._IOBase for module state (python#104386)
  pythongh-104413: Fix refleak when super attribute throws AttributeError (python#104414)
  Fix refleak in `super_descr_get` (python#104408)
  pythongh-87526: Remove dead initialization from _zoneinfo parse_abbr() (python#24700)
  pythongh-91896: Improve visibility of `ByteString` deprecation warnings (python#104294)
  pythongh-104371: Fix calls to `__release_buffer__` while an exception is active (python#104378)
  pythongh-104377: fix cell in comprehension that is free in outer scope (python#104394)
  pythongh-104392: Remove _paramspec_tvars from typing (python#104393)
  pythongh-104396: uuid.py to skip platform check for emscripten and wasi (pythongh-104397)
  pythongh-99108: Refresh HACL* from upstream (python#104401)
  pythongh-104301: Allow leading whitespace in disambiguated pdb statements (python#104342)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Eclips4 Eclips4 Eclips4 left review comments
@hauntsaninja hauntsaninja hauntsaninja approved these changes
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-typing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@AlexWaygood @rhettinger @hauntsaninja @Eclips4 @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.