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

Enable the Nullable project property and fix all the warnings #194

Merged
merged 15 commits into from
Feb 6, 2023
Merged

Enable the Nullable project property and fix all the warnings #194

merged 15 commits into from
Feb 6, 2023

Conversation

dberry-rcs
Copy link
Contributor

Enabled nullable for just the MongoDB.Entities project. Fixed all the warnings that resulted from the change.

@dj-nitehawk
Copy link
Owner

made some more improvements with regards to nullables. could you have a quick glance over and see if everything looks ok pls?
if all good, i'll merge to master tomorrow.
i think we will be making the next release a major version jump since there's a possibility this may break some existing code (even though easily fixable).
after merging the PR, i'll release a beta version so we could take it for a good spin before a public release.
thanks for this! been procrastinating enabling nullable support for a long time ;-)

@dberry-rcs
Copy link
Contributor Author

Cleaned up some warnings that Rider was showing.

@dj-nitehawk
Copy link
Owner

okieee... did some more nullable improvements...
what does rider say now?
i think we're almost there :-)

@dberry-rcs
Copy link
Contributor Author

Resolved the last of the warnings

@dj-nitehawk dj-nitehawk merged commit 2df963c into dj-nitehawk:master Feb 6, 2023
@dj-nitehawk
Copy link
Owner

good job...
thanks a lot for this...
definitely was a needed change to move the library forward :-)

@dj-nitehawk
Copy link
Owner

pushed a beta release to nuget v21.0.2.1-beta

@dj-nitehawk
Copy link
Owner

dj-nitehawk commented Feb 6, 2023

was just testing out the beta in an existing project and now i'm second guessing the choice to make the IEntity.ID nullable.

now i'm thinking that the only time it would be null is when the user instantiates an instance themselves and right up until it is saved to the db.

so what are the chances that user code would be trying to read the ID property before the entity is saved. 99% of the time when an entity is retrieved from the db, it would not be null, unless the query code explicity does a projection to exclude the ID. in which case the developer knows it would be null and would take steps to do a null check. or most likely the ID property would not be accessed at all in his code, which is why it was excluded in the first place.

what are your thoughts on this?
shall i make the ID property non-nullable?

@dberry-rcs
Copy link
Contributor Author

dberry-rcs commented Feb 6, 2023

I upgraded my existing project to the beta. I had 13 errors to start with and I had to "!" all the ids that I knew would be filled in.

While I prefer IDs that are not null, the fact that there are 2 use cases where the id can be null means that it should be nullable. It is up to the user of the library to determine if they feel comfortable enough to use "!" on the id. I would rather do this than get a NullPointer Exception at runtime.

@dberry-rcs dberry-rcs deleted the nullable branch February 6, 2023 13:21
@dj-nitehawk
Copy link
Owner

okiee then... we'll leave the ID as nullable...
if you find anymore nullable fixes that are needed, pls send in a new PR.
tc mate!

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