HypothesisWorks / hypothesis

Hypothesis is a powerful, flexible, and easy to use library for property-based testing.
https://hypothesis.works
Other
7.58k stars 587 forks source link

Add better support for pretty-printing record types #4133

Closed DRMacIver closed 1 month ago

DRMacIver commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

tybug commented 1 month ago

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 commented 1 month ago

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

DRMacIver commented 1 month ago

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?

DRMacIver commented 1 month ago

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.