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:@sydney-runkle

Remove guarding check on computed field with field_serializer

c7b035a

github-actionsbot added the relnotes-fixUsed for bugfixes. label

Sep 12, 2024

Copy link

ContributorAuthor

nix010 commentedSep 12, 2024

please review

pydantic-hookybot added the ready for review label

Sep 12, 2024

pydantic-hookybot assignedsydney-runkle

Sep 12, 2024

Copy link

codspeed-hqbot commentedSep 12, 2024•
edited
Loading

CodSpeed Performance Report

Merging#10390 willnot alter performance

_{Comparingnix010:9683-computed-field-and-field-serializer (4be672b) withmain (a6dc872)}

Summary

✅ 49 untouched benchmarks

Fix test custom_serializer

3529921

Copy link

Contributor

github-actionsbot commentedSep 12, 2024•
edited
Loading

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic/_internal
_decorators.py
_generate_schema.py
Project Total

_{This report was generated bypython-coverage-comment-action}

sydney-runkle reviewed

Sep 12, 2024

View reviewed changes

tests/test_serialize.py Outdated

		def two_x(self) -> int:
		return self.x * 2
		@field_serializer('two_x')
		def ser_two_x_bad_signature(self, v, _info):

Copy link

Contributor

sydney-runkleSep 12, 2024

Choose a reason for hiding this comment

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

Should probably rename this - doesn't have a bad signature now.

Copy link

Contributor

sydney-runkleSep 12, 2024

Choose a reason for hiding this comment

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

Can probably delete this test honestly - not sure of the value here anymore, especially given the other test added above.

sydney-runkle suggested changes

Sep 12, 2024

View reviewed changes

Copy link

Contributor

sydney-runkle left a comment

Choose a reason for hiding this comment

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

A few change requests, thanks!

tests/test_serialize.py Outdated

		def two_x(self) -> int:
		return self.x * 2
		@field_serializer('two_x')
		def ser_two_x_bad_signature(self, v, _info):

Copy link

Contributor

sydney-runkleSep 12, 2024

Choose a reason for hiding this comment

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

Can probably delete this test honestly - not sure of the value here anymore, especially given the other test added above.

pydantic-hookybot added awaiting author revisionawaiting changes from the PR author and removed ready for review labels

Sep 12, 2024

pydantic-hookybot assignednix010 and unassignedsydney-runkle

Sep 12, 2024

sydney-runkle added relnotes-feature and removed awaiting author revision

awaiting changes from the PR author

relnotes-fixUsed for bugfixes. labels

Sep 12, 2024

Copy link

Contributor

sydney-runkle commentedSep 12, 2024

LGTM overall - happy to merge after we fix up the tests a bit.

Confirming with the team that there was no reason not to support this, before we merge as well.

Copy link

Contributor

sydney-runkle commentedSep 12, 2024•
edited
Loading

See#6965 where this was added - there's some other relevant code that we should update. We can remove thecomputed_field: bool from the_apply_field_serializers function, as well :).

sydney-runkle added the awaiting author revisionawaiting changes from the PR author label

Sep 12, 2024

nix010 added2 commits

September 13, 2024 10:48

Remove signature computed_field

9200ba1

Merge branch 'main' into 9683-computed-field-and-field-serializer

4be672b

Copy link

ContributorAuthor

nix010 commentedSep 13, 2024

@sydney-runkle removed. please review again.

See#6965 where this was added - there's some other relevant code that we should update. We can remove thecomputed_field: bool from the_apply_field_serializers function, as well :).