Introducing classproperty decorator for model_computed_fields

[Jocelyn-Gas]

Change Summary

A classproperty decorator has been added in order to access model_computed_fields as a class attribute and an instance property.

Related issue number

fix #7986

Checklist

• The pull request title is a good summary of the changes - it will be used in the changelog
• Unit tests for the changes exist
• Tests pass on CI
• Documentation reflects the changes where applicable
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @samuelcolvin

[Jocelyn-Gas]

Please review the above PR to check it suits the need you described @hottwaj @sydney-runkle

[codspeed-hq]

CodSpeed Performance Report

Merging #8437 will not alter performance

_{Comparing Jocelyn-Gas:model-computed-field-classproperty (b36fca8) with main (19fa86c)}

Summary

✅ 10 untouched benchmarks

[Jocelyn-Gas]

It's my first contribution attempt to this repo. I'm a bit surprised but the release notes check that is not passing. If anyone can help me please, I'd be more than happy to fix it if pointed in the right direction 😁

[Viicos]

I would also check what happens when trying to set the model_computed_fields attribute to some value (for reference, having the chain of @classmethod + @property was deprecated in 3.11 partly because of unsupported behavior when setting values on those properties).

[Jocelyn-Gas]

Is it supposed to be settable ?

[Viicos]

I'm a bit surprised but the release notes check that is not passing. If anyone can help me please, I'd be more than happy to fix it if pointed in the right direction 😁

I think this is just a matter of maintainers adding a label on this PR

[hottwaj]

Hey while I'm not the right person to comment on what is the best choice for pydantic, I just wanted to offer my opinion that I'm not sure about adding a new decorator class (classproperty) just for the purpose of this one-off usage for model_computed_fields. Doesn't this overcomplicate things? I think it would be better to look at what is done when __pydantic_decorators__ is set and set model_computed_fields at the same time? Thanks!

…

On Mon, 25 Dec 2023 at 21:40, Viicos ***@***.***> wrote: I'm a bit surprised but the release notes check that is not passing. If anyone can help me please, I'd be more than happy to fix it if pointed in the right direction 😁 I think this is just a matter of maintainers adding a label on this PR — Reply to this email directly, view it on GitHub <#8437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABK6JBLEGRNQ6GP4TZYSNCDYLHXDVAVCNFSM6AAAAABBB2DAL6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRZGEZDKMRWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Viicos]

I'm not sure about adding a new decorator class (classproperty) just for the purpose of this one-off usage for model_computed_fields.

I kind of agree, taking a look at it it seems this can be set during class construction (this property doesn't make use of any "dynamic" attribute that could change).

[Jocelyn-Gas]

Thanks for the review! I agree with you guys! I just removed the @classproperty descriptor for a class attribute that is set in the metaclass.

[hottwaj]

Thanks! The new class attribute implementation looks good. In reply to your question about model_computed_fields being settable, I think that setting model_computed_fields would be unusual user behaviour and should be destructive as usual i.e. remove any existing attribute. I'm not sure any test is needed for that? I think Viicos' comment was more for when the classproperty implementation was being used. Thanks!

…

On Tue, 26 Dec 2023 at 18:16, Jocelyn-Gas ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/test_computed_fields.py <#8437 (comment)>: > + assert Rectangle.model_computed_fields['area2'].title == 'Pikarea' + assert Rectangle.model_computed_fields['area2'].description == 'Another area' Is it supposed to be settable ? — Reply to this email directly, view it on GitHub <#8437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABK6JBO6IXDSPPFWSH5K6NTYLMIABAVCNFSM6AAAAABBB2DAL6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJWGUYTSNRYG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Viicos]

I think Viicos' comment was more for when the classproperty implementation was being used.

Indeed, this attribute shouldn't be set and with classproperty I wanted to make sure it was the case.

[sydney-runkle]

@Jocelyn-Gas,

This looks great, thanks a ton for your contribution.

Apologies for the delayed review - things got a bit backlogged with the holidays.

[Jocelyn-Gas]

@Jocelyn-Gas,

This looks great, thanks a ton for your contribution.

Apologies for the delayed review - things got a bit backlogged with the holidays.

No worries for the delay, holidays first!

I'm glad I could help, this was a first on this project! 😁