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

Allow #[diesel(treat_none_as_{null, default_value} = …] on fields #3724

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

moulins
Copy link
Contributor

@moulins moulins commented Aug 1, 2023

If present on a field, this will override the corresponding container-level attribute.

This restores feature parity with the old diesel 1.x versions, which allowed these two attributes on struct fields.

@weiznich weiznich requested a review from a team August 2, 2023 06:15
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Need tests, both happy and error cases.

@Ten0 Ten0 self-requested a review August 2, 2023 09:12
@moulins
Copy link
Contributor Author

moulins commented Aug 2, 2023

I've added/modified tests to exercise field-level #treat_none_as_null, and to reject invalid embed, treat_none_as_default_value combinations.

However, I didn't see any pre-existing tests for treat_none_as_default_value to modify and I'm not sure how it should be tested; what should be done here?

@moulins
Copy link
Contributor Author

moulins commented Aug 2, 2023

Something I just thought about: as implemented, putting an explicit treat_none_as_null on a non-Option field will do nothing; maybe it should be an error instead?

@Ten0
Copy link
Member

Ten0 commented Aug 2, 2023

maybe it should be an error instead

yes that makes sense, that would be better

@weiznich weiznich force-pushed the derive-fieldwise-opts branch from a7b99ae to bb707bf Compare August 3, 2023 13:49
@weiznich
Copy link
Member

weiznich commented Aug 3, 2023

I've rebased this branch to pull in the changes from #3729 to fix the broken tests.

@weiznich weiznich enabled auto-merge August 3, 2023 13:54
@weiznich weiznich added this pull request to the merge queue Aug 3, 2023
Merged via the queue into diesel-rs:master with commit 7e2cd73 Aug 3, 2023
@moulins moulins deleted the derive-fieldwise-opts branch August 3, 2023 16:11
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.

4 participants