GeezerVison

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

GeezerVison is showing github.com — bpo-33805: Improve error message of dataclasses.replace() by corona10 · Pull Request #7580 · python/cpython

Conversation

corona10 commented Jun 10, 2018 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

https://bugs.python.org/issue33805

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 requested a review from ericvsmith as a code owner

June 10, 2018 06:15

the-knights-who-say-ni added the CLA signed label

Jun 10, 2018

bedevere-bot added the awaiting review label

Jun 10, 2018

corona10 commented Jun 10, 2018

Copy link

Copy Markdown

Member Author

@ericvsmith PTAL

All reactions

Sorry, something went wrong.

Uh oh!

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

ericvsmith requested changes

Jun 10, 2018

View reviewed changes

ericvsmith left a comment

Copy link

Copy Markdown

Member

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 for the PR. See comments.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/dataclasses.py Outdated

ericvsmith Jun 10, 2018

Copy link

Copy Markdown

Member

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.

I don't think it's worth adding this mapping and the new __str__ just for this one error message.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/dataclasses.py Outdated

ericvsmith Jun 10, 2018

Copy link

Copy Markdown

Member

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.

For consistency with the rest of the code, please change '{f.name}' to {f.name!r}. Also, just hard-code InitVar in the message, since this message is only applicable with InitVars.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/dataclasses.py Outdated

ericvsmith Jun 10, 2018

Copy link

Copy Markdown

Member

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.

The else is not needed.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_dataclasses.py Outdated

ericvsmith Jun 10, 2018

Copy link

Copy Markdown

Member

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.

Please add a test that shows that an InitVar does work with replace(), and check that its value is used (probably via a __post_init__ method). Maybe something like:

def __post_init__(self, y):
   self.x *= y

or similar.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Misc/NEWS.d/next/Library/2018-06-10-15-14-17.bpo-33805.5LAz5a.rst Outdated

ericvsmith Jun 10, 2018

Copy link

Copy Markdown

Member

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.

I'd prefer "Improve error message of dataclasses.replace() when an InitVar is not specified".

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot commented Jun 10, 2018

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting changes and removed awaiting review labels

Jun 10, 2018

corona10 force-pushed the bpo-33805 branch from a46c10d to 3248d9c Compare

June 10, 2018 13:22

corona10 changed the title ~~bpo-33805: Update error message of dataclasses.replace()~~ bpo-33805: Improve error message of dataclasses.replace()

Jun 10, 2018


          bpo-33805: Improve error message of dataclasses.replace()

22aee12

corona10 force-pushed the bpo-33805 branch from 3248d9c to 22aee12 Compare

June 10, 2018 13:55

corona10 commented Jun 10, 2018

Copy link

Copy Markdown

Member Author

@ericvsmith
I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot commented Jun 10, 2018

Copy link

Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting change review and removed awaiting changes labels

Jun 10, 2018

ericvsmith commented Jun 10, 2018

Copy link

Copy Markdown

Member

Thanks! Looks good.

I'm going to wait a few days before I merge this, because at this late date I don't want to put this in to 3.7.0, I'll wait for 3.7.1.

corona10 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

ericvsmith added the needs backport to 3.7 label

Jun 23, 2018

ericvsmith approved these changes

Jun 23, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels

Jun 23, 2018

ericvsmith merged commit 3d70f7a into python:master

Jun 23, 2018

miss-islington commented Jun 23, 2018

Copy link

Copy Markdown

Contributor

Thanks @corona10 for the PR, and @ericvsmith for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the awaiting merge label

Jun 23, 2018

bedevere-bot commented Jun 23, 2018

Copy link

Copy Markdown

GH-7876 is a backport of this pull request to the 3.7 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the needs backport to 3.7 label

Jun 23, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jun 23, 2018


          bpo-33805: Improve error message of dataclasses.replace() (pythonGH-7580

)

(cherry picked from commit 3d70f7a)

Co-authored-by: Dong-hee Na <[email protected]>

miss-islington added a commit that referenced this pull request

Jun 23, 2018


          bpo-33805: Improve error message of dataclasses.replace() (GH-7580)

bbef7ab

(cherry picked from commit 3d70f7a)

Co-authored-by: Dong-hee Na <[email protected]>

corona10 deleted the bpo-33805 branch

June 25, 2018 04:50

noamkush mentioned this pull request

Dec 13, 2018

Merge changes from bpo-33796 and bpo-33805 ericvsmith/dataclasses#140

Merged

Reviewers

ericvsmith

ericvsmith approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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

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.

GeezerVison

Conversation

corona10 commented Jun 10, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Jun 10, 2018

Uh oh!

ericvsmith left a comment

Choose a reason for hiding this comment

Uh oh!

ericvsmith Jun 10, 2018

Choose a reason for hiding this comment

Uh oh!

ericvsmith Jun 10, 2018

Choose a reason for hiding this comment

Uh oh!

ericvsmith Jun 10, 2018

Choose a reason for hiding this comment

Uh oh!

ericvsmith Jun 10, 2018

Choose a reason for hiding this comment

Uh oh!

ericvsmith Jun 10, 2018

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 10, 2018

Uh oh!

corona10 commented Jun 10, 2018

Uh oh!

bedevere-bot commented Jun 10, 2018

Uh oh!

ericvsmith commented Jun 10, 2018

Uh oh!

miss-islington commented Jun 23, 2018

Uh oh!

bedevere-bot commented Jun 23, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

corona10 commented Jun 10, 2018 •

edited by bedevere-bot

Loading

Uh oh!

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