FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-142214: Fix two regressions in dataclasses by JelleZijlstra · Pull Request #142223 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-142214: Fix two regressions in dataclasses - #142223

Merged
JelleZijlstra merged 7 commits into
python:mainfrom
JelleZijlstra:fixdatacl
Dec 5, 2025
ConversationCommits (7)ChecksFiles changed
Merged

gh-142214: Fix two regressions in dataclasses#142223
JelleZijlstra merged 7 commits into
python:mainfrom
JelleZijlstra:fixdatacl

Conversation

JelleZijlstra commented Dec 3, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Dec 3, 2025
Closed
AlexWaygood reviewed Dec 3, 2025
View reviewed changes
Comment thread Lib/dataclasses.py Outdated
sobolevn reviewed Dec 3, 2025
View reviewed changes
Comment thread Lib/dataclasses.py Outdated

DavidCEllis commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

I am kicking myself on the __annotate__ access. Honestly surprised it didn't trip any existing tests.

It looks like the sqlalchemy field issue is from replacing and subsequently attempting to restore annotations in order to add fields to a dataclass as there's no mechanism for doing so without annotations. The check to fix this is fine, I'm actually surprised I hadn't written this as a comprehension.

Again, sorry for missing these possibilities.

Update Lib/dataclasses.py
1051d6c 
Co-authored-by: sobolevn <mail@sobolevn.me>
basnijholt approved these changes Dec 3, 2025
View reviewed changes

basnijholt left a comment •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I just locally applied the changes in Lib/dataclasses.py and it fixed all the failing tests on github.com/pipefunc/pipefunc.

JelleZijlstra added the needs backport to 3.14 bugs and security fixes label Dec 3, 2025
DavidCEllis mentioned this pull request Dec 3, 2025
Closed
ericvsmith requested changes Dec 3, 2025
View reviewed changes

ericvsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A small nit, but otherwise looks good to me.

Comment thread Lib/dataclasses.py
# gh-142214: The annotation may be missing in unusual dynamic cases.
# If so, just skip it.
if k in cls_annotations:
new_annotations[k] = cls_annotations[k]

ericvsmith Dec 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Don't in and [] both do a lookup? Maybe it would be better as:

if ann := cls_annotations.get(k):
    new_annotations[k] = ann

DavidCEllis Dec 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I believe that None is a valid annotation here so this would miss None values?

JelleZijlstra Dec 3, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Correct. To save a lookup we could do a try-except KeyError instead. I don't have a preference but I can switch to that if you like.

DavidCEllis Dec 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think try/except makes sense as in most cases the key should be there. The key not being there should only be in cases where something has modified annotations and removed things after the class has been processed.

JelleZijlstra Dec 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done

bedevere-app Bot commented Dec 3, 2025

Copy link
Copy Markdown

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

hugovk mentioned this pull request Dec 4, 2025
Closed

JelleZijlstra commented Dec 4, 2025

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

bedevere-app Bot commented Dec 4, 2025

Copy link
Copy Markdown

Thanks for making the requested changes!

@ericvsmith: please review the changes made to this pull request.

bedevere-app Bot requested a review from ericvsmith December 4, 2025 19:50
mweinelt mentioned this pull request Dec 5, 2025
Merged
4 of 13 tasks
ericvsmith approved these changes Dec 5, 2025
View reviewed changes

ericvsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me. Thanks!

JelleZijlstra merged commit 53ec7c8 into python:main Dec 5, 2025
51 of 54 checks passed

miss-islington-app Bot commented Dec 5, 2025

Copy link
Copy Markdown

Thanks @JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 5, 2025
pythongh-142214: Fix two regressions in dataclasses (pythonGH-142223)
498e477 
(cherry picked from commit 53ec7c8)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

bedevere-app Bot commented Dec 5, 2025

Copy link
Copy Markdown

GH-142277 is a backport of this pull request to the 3.14 branch.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Dec 5, 2025
JelleZijlstra deleted the fixdatacl branch December 5, 2025 04:06
JelleZijlstra added a commit that referenced this pull request Dec 5, 2025
[3.14] gh-142214: Fix two regressions in dataclasses (GH-142223) (#14…
654e3c7 
…2277)

gh-142214: Fix two regressions in dataclasses (GH-142223)
(cherry picked from commit 53ec7c8)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
pythongh-142214: Fix two regressions in dataclasses (python#142223)
3265f88
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sobolevn sobolevn left review comments
DavidCEllis DavidCEllis left review comments
AlexWaygood AlexWaygood left review comments
ericvsmith ericvsmith approved these changes
+1 more reviewer
basnijholt basnijholt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL