Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better support for pretty-printing record types #4133

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

DRMacIver
Copy link
Member

This PR adds logic so that attrs and dataclass defined classes get pretty printed directly via their repr.

Use case: They mostly pretty print fine with reprs, but I wanted to define custom pretty printing for a type that was contained in a dataclass, which doesn't work if you rely on repr printing.

I've made this a minor release because it changes user visible behaviour in a way that we have no compatibility requirements on but that is arguably undesirable in some cases. In particular if a type is defined this way but has a custom repr, we will now ignore that repr. There's plenty of precedent for us doing that, and the logic of when we should use the repr after all is quite fiddly, so I think this is OK.

@tybug
Copy link
Member

tybug commented Oct 9, 2024

In particular if a type is defined this way but has a custom repr, we will now ignore that repr

I dunno about this. Some users may define a custom repr that intentionally removes some attribute (eg because of a very large repr), and then get annoyed when hypothesis prints it anyway. More generally, if a user has gone to the effort to define a custom repr on a dataclass, we should probably respect it?

@dataclass
class A:
    x: object
    def __repr__(self):
        return "myrepr"

pretty.pretty(A(x=1)) # myrepr on master, A(x=1) on this branch

@DRMacIver
Copy link
Member Author

I dunno about this. Some users may define a custom repr that intentionally removes some attribute (eg because of a very large repr), and then get annoyed when hypothesis prints it anyway.

Hmm so in both attrs and dataclasses you can mark an attribute as excluded from the repr, which I expect to be the main thing people use for this, and you're right that should probably be respected.

More generally, if a user has gone to the effort to define a custom repr on a dataclass, we should probably respect it?

In general, Hypothesis doesn't respect custom reprs (my initial attempt at this accidentally made it so that it did this for all classes, at which point I found out quite how many reprs are overridden) and I think it's mostly correct not to. Hypothesis's pretty printing is designed to do something a bit different from a typical repr in that it's supposed to be code that reproduces the value, which is not something that reprs reliably do, and using custom reprs is much worse than using pretty printing because it will tend to call the repr of any object contained within it, which although in theory is a thing Python reprs are designed to do, it's a theory that's routinely violated in practice.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 10, 2024

Overall seems like a good idea to me. Interactions to check:

  • non-init arguments shouldn't be included
  • I don't think we should respect exclude-from-repr if that's an argument that has to be passed in the call. I could probably be argued around to excluding if the repr would be >1k characters or something though.
  • just check that this is lower priority than the current known-call override

@tybug
Copy link
Member

tybug commented Oct 10, 2024

Hypothesis's pretty printing is designed to do something a bit different from a typical repr in that it's supposed to be code that reproduces the value

Point taken! Objection withdrawn for the failing example.

This might have knock-on effects for other things, like to_jsonable and therefore observability. (though we handle dataclasses/attrs specially in to_jsonable, so maybe not an issue?)

@Zac-HD
Copy link
Member

Zac-HD commented Oct 10, 2024

to_jsonable seems fine, it's operating on the values and only falls back to pretty-printing if that fails.

@DRMacIver
Copy link
Member Author

non-init arguments shouldn't be included

Good point!

I don't think we should respect exclude-from-repr if that's an argument that has to be passed in the call. I could probably be argued around to excluding if the repr would be >1k characters or something though.

Yeah, this seems like a reasonable position to me.

just check that this is lower priority than the current known-call override

Sorry, could you clarify? This is the how-to-generate printing for explore phase?

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

I'm happy to merge this, with the addition of some skipif-py314 markers - just leave a FIXME-py314 comment so we'll fix them in the betas next year (#4028)

@DRMacIver
Copy link
Member Author

just leave a FIXME-py314 comment so we'll fix them in the betas next year

Sounds good. I'm like 80% sure fixing them in the betas will just involve waiting for attrs to support the betas and then removing the comments. It looks like something in attrs not working properly rather than anything we're specifically doing.

@DRMacIver DRMacIver force-pushed the DRMacIver/better-record-printing branch from 2dba950 to a302e36 Compare October 12, 2024 08:52
@Zac-HD Zac-HD merged commit 857c3c9 into master Oct 12, 2024
51 checks passed
@Zac-HD Zac-HD deleted the DRMacIver/better-record-printing branch October 12, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants