Improve nullable annotations to correctly reflect what is nullable and what is not
Closed this issue · 3 comments
Proposed change
Apply the following changes across all serializable models:
- Remove any assignment of
default!(which kills any nullable warnings) - Mark nullable properties as such
- Add the
requiredkeyword to any required fields - Mark JSON constructor with [SetsRequiredMembers]
Use case
I was trying to handle the PubAckResponse you get back after publishing to a JetStream, and the Error property is marked as not-null. However, it can totally be null (the corresponding unit tests also check for this).
This is a problem for the consuming code, as any check for null on such a property results in a compiler warning (stating that the check is superfluous) and squiggles that aren't nice to look at. Removing the null check is also not nice, for obvious reasons.
It's also hard to discover which properties need to be null checked when they aren't annotated correctly.
Contribution
Yes, can submit a PR, but will need guidance on which properies are actually nullable or not (as many of these messages are generated outside of the code). If there is a reference spec on the message formats underlying the models then that'll work too.
we can work through it case by case based on the schemas and potentially checking the server code. Please feel free to kick off the PR.
I may need a few days to get started on this (festivities and all) but will return when I have something to look at.