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

Bugfix for https://github.com/mbdavid/LiteDB/issues/2385 Also adds full record support by fixing this bug. #2384

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

athendrix
Copy link
Contributor

In theory, this should also work for any class that doesn't have an empty constructor, but does have a constructor that matches the properties in names and types.

To use the [BsonId] attribute on a record property, you need to apply it to the property, not the constructor value. So you use [property:BsonId] instead.

public record Customer([property:BsonId] Guid PoorlyNamedId, string Name, string[] Phones, bool IsActive);

But if you just name it the way LiteDB expects, you don't even need to do that much.

public record Customer(Guid CustomerId, string Name, string[] Phones, bool IsActive);

To use the [BsonId] attribute in a record, you need to apply it to the property, not the constructor value. So you use [property:BsonId] instead.
@athendrix
Copy link
Contributor Author

My new code passes all the tests, and still works for records.
It just fixes the logic for constructor finding.

@athendrix
Copy link
Contributor Author

This fixes bug #2385 (test included)

@athendrix athendrix changed the title Added full record support. Bugfix for https://github.com/mbdavid/LiteDB/issues/2385 Also adds full record support by fixing this bug. Nov 10, 2023
@athendrix
Copy link
Contributor Author

@mbdavid What would you say the are the odds this will get merged at some point? Was the master branch the wrong place to submit these kinds of fixes?

For what it's worth, it's only one function swapped out, I even added a test to illustrate the bug being fixed that fails on the master branch but succeeds in my branch, and I'm sure plenty of people would be happy to be able to have records work as well as classes, even if it weren't fixing an actual bug.

Also, feel free to change my coding style, since it's fairly obviously different before merging.

@mbdavid
Copy link
Owner

mbdavid commented Nov 29, 2023

Hi @athendrix, thanks for your contribution. I'm reading your code now to undersant better your idea. I also will fix your code style, ok?

@mbdavid mbdavid merged commit d6f0ca4 into mbdavid:master Nov 29, 2023
1 check passed
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.

2 participants