bpo-43080: pprint for dataclass instances by LewisGaul · Pull Request #24389 · python/cpython

I've realised this doesn't respect user-defined __repr__() methods. I wasn't able to use the existing mechanism of comparing the IDs of __repr__ in the _dispatch dict as dynamically created __repr__s all have different IDs.

The best solution to this I can think of is to check object.__repr__.__module__ - this seems to be 'dataclasses' if defined by the dataclass machinery. I'm not sure how watertight this solution is though?

EDIT: This solution doesn't work on master actually...

This PR is stale because it has been open for 30 days with no activity.

I'm still not convinced this is an awesome idea, but see my review comments anyway.

I'm happy to address the comments, but if we're not expecting it to be merged shall we just close the PR? I would like to see this feature make it into core, but I won't be offended if you're against it overall, and better not to waste time fixing stuff up if it's not going in anyway! :)

I've realised this doesn't respect user-defined __repr__() methods. I wasn't able to use the existing mechanism of comparing the IDs of __repr__ in the _dispatch dict as dynamically created __repr__s all have different IDs.

I've added a failing testcase to track this issue.

@ericvsmith Do you have any thoughts on how best to tackle this? You suggested the following on the issue, is this approach worth pursuing here?

Adding an attribute on the __repr__ (and other methods?) signifying that they were generated would let us distinguish them.

Say, checking getattr(__repr__, '__is_generated__', False), or similar.

For the __repr__ check, how about examining __repr__.__qualname__ and checking for __create_fn__.<locals>.__repr__? I think that's a sufficiently unlikely string that it could be used. Although it's an implementation detail and probably a fragile test, I'd be okay with using it inside the stdlib. Or we could change the name of __create_fn__ to __dataclasses_create_fn__ to reinforce the fact that this is a dataclasses internal.

For the __repr__ check, how about examining __repr__.__qualname__ and checking for __create_fn__.<locals>.__repr__? I think that's a sufficiently unlikely string that it could be used. Although it's an implementation detail and probably a fragile test, I'd be okay with using it inside the stdlib. Or we could change the name of __create_fn__ to __dataclasses_create_fn__ to reinforce the fact that this is a dataclasses internal.

I see what you're suggesting when trying it out on 3.8, but it seems the qualname was 'fixed' in #22155, so this approach won't work on master. Any other ideas/preferences? :)

I see what you're suggesting when trying it out on 3.8, but it seems the qualname was 'fixed' in #22155, so this approach won't work on master. Any other ideas/preferences? :)

You could do the same with __repr__.__wrapped__:

<function __create_fn__.<locals>.__repr__ at 0x0000022FF4824670>

Check that __wrapped__ exists and contains "__create_fn__".

Again, it's super fragile, but since both modules are in the stdlib and will be tested to work together, I'm not so concerned.

Again, it's super fragile, but since both modules are in the stdlib and will be tested to work together, I'm not so concerned.

And you can blame me in a comment if you want!

@ericvsmith this should be good for second review/merge now :)

@LewisGaul : I'm planning on fixing a bug (which I can't find right now) dealing with repr of recursive dataclasses. I'm going to hold off on this until that one is merged, at which point we can do the same fix here. But if you haven't heard from me in a week or so, please ping me again.

@ericvsmith I've added handling for recursive dataclasses. Note that this required some special-casing (to avoid backwards incompatibility of formatting other recursive objects), where the existing handling in the pprint module would have given the following:

dataclass4(a=<Recursion on dataclass4 with id=2119600742448>)

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.

I have made the requested changes; please review again.

I don't think the CI failures are related (test_ssl failure on macOS, docs build failure).

Thanks for making the requested changes!

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

Thanks for taking the time to review @ericvsmith :) Is there anything else I need to do before this can be merged?

@LewisGaul : No, nothing else. I'll try and finish up my final review and push of this tonight. Thanks for your patience.