bpo-41747: Ensure all dataclass methods uses their parents' qualname by isidentical · Pull Request #22155 · python/cpython

isidentical

Choose a reason for hiding this comment

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

This LGTM but it would be great if @ericvsmith could take a final look

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.

Choose a reason for hiding this comment

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

Seems this change also makes these methods pickleable.

		# Never overwrites an existing attribute. Returns True if the
		# attribute already exists.
		if isinstance(value, FunctionType):
		value.__qualname__ = f"{cls.__qualname__}.{value.__name__}"

Choose a reason for hiding this comment

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

Why value.__name__ and not name? Are they always the same?

Choose a reason for hiding this comment

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

For all use cases, but since it got extracted to another function which only takes cls and value, I guess there is no need to complicate that function.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

With a few tiny issues, this looks good to me.

@serhiy-storchaka: Should we also add tests for pickling these methods? Or should that be another PR?

I do not think that it is necessary to add tests for pickling these methods.

Sorry for the delays. This is on my list of things to look at next week during the development sprints.

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 13, 2021

…ythonGH-22155)

* bpo-41747: Ensure all dataclass methods uses their parents' qualname

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>