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 support for ValueTypes that consist only of Fields #2571 #2572

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JKamsker
Copy link
Collaborator

@JKamsker JKamsker commented Dec 2, 2024

Fixes #2570
Also fixes issues in #2568

@Joy-less One last opinion pretty please? :)

@JKamsker JKamsker requested a review from Joy-less December 2, 2024 08:48
@Joy-less
Copy link
Collaborator

Joy-less commented Dec 2, 2024

From what I can tell, this is just because BsonMapper sets IncludeFields to false by default. So the behaviour is functioning correctly. It would make more sense imo to set IncludeFields to true by default or to put a warning that ValueTuple uses fields not properties.

@JKamsker
Copy link
Collaborator Author

JKamsker commented Dec 3, 2024

Hi Joy-Less,

Thank you for your feedback! The goal of this PR is to make serialization more intuitive while preserving current defaults. Classes typically rely on properties to define their data, so the default behavior (IncludeFields = false) makes sense to avoid unintentionally serializing fields. Structs, on the other hand, often use fields for their data, and this change ensures field-only structs like ValueTuple are handled correctly. For structs with properties, the logic still respects properties as the primary data source.

I also have the feeling that the library was developed with classes in mind and didn’t fully consider how structs might be used. This PR addresses that gap by improving support for structs while maintaining compatibility with the original design philosophy.

Changing IncludeFields to true by default could break existing applications, so this PR carefully enhances support for specific cases like field-only value types without altering established behavior. I believe this strikes a good balance between stability and usability.

Looking forward to your thoughts!

@Joy-less
Copy link
Collaborator

Joy-less commented Dec 3, 2024

I understand. Please write a comment in the source code clearly explaining this reasoning, then I will approve.

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.

[BUG] ValueTuples are not serialized correctly
2 participants