Nullable reference types for server projects

[benbryant0]

Select Topic Area

✅ Code Contribution Proposal

Code Contribution Proposal

Hi! I just grabbed the server repo and noticed it's not using nullable reference types much, and then found this relevant ADR.

I see the decision was "Enabled project-wide, disable at the file level". Is this something I can contribute to? If so, is there a preferred way to go about it? E.g. one PR per project vs one monster PR.

[github-actions[bot]]

✨ Thank you for your code contribution proposal! While the Bitwarden team reviews your submission, we encourage you to check out our contribution guidelines.

Please ensure that your code contribution includes a detailed description of what you would like to contribute, along with any relevant screenshots and links to existing feature requests. This information helps us gather feedback from the community and Bitwarden team members before you start writing code.

To keep discussions focused, posts that do not include a proposal for a code contribution will be removed.

• 💡 For feature requests and general discussion, please visit the Bitwarden Community Forums.
• ℹ️ For questions and support, visit the Bitwarden Help Center.
• 🐛 To report a potential bug, please visit the corresponding repo. Server | Clients | iOS | Android

Thank you for contributing to Bitwarden!

[justindbaur]

Hi @benbryant0, thank you so much for the interest in helping! While writing that ADR I did write some tooling to help with the migration, I hadn't posted it anywhere so I uploaded it to my personal GitHub in case you want to take a look here. Essentially it is just a custom logger so I can get a list of files that have an issue with the nullable migration and then I just add #nullable disable to the top of each file. We have a CODEOWNERS file in the server repo so doing all those changes would get what we call "review bingo" and for that I've actually built another tool to help make team specific commits and can then open a PR that only hits a single team.

So if/when that ADR is accepted by the other stakeholders here at Bitwarden I was going to put that plan into place. If you're interested in doing the migration with the above tooling, manually, or your own way I would be more than willing to notify you on here that the ADR has been accepted and I can help you get those PR's in.

If you want to do some nullable based contributions now you can just pick any file add #nullable enable in it and fix the issues that arise from them to the best of your ability and then ping me on the PR if you have any questions. Or if you want to wait until we have most of the code migrated you will be able to search the codebase for where we've had to temporarily disable nullable and help migrate those to be compliant.

[benbryant0]

Hi @justindbaur. Thanks for the info, that's helpful. I just went ahead and got the disabling-per-file done & building for 6 projects since it didn't take long, but the issue of review bingo totally makes sense. I'll just shelve that for now until everybody is on board with the ADR, then split it by ownership like you suggested. Thanks for the tooling for that!

Until then, I'll definitely do some explicit enabling per file. I assume it'd make sense to keep the files changed in any single PR grouped by team ownership as well?

[justindbaur]

Looks like you got started before I could reply!

I think single team PR's are much easier to get merged but it's by no means a rule and if you find it easier to create one PR then I will do my best to rally the teams to get reviews on it so we can get it though!

[benbryant0]

Yeah, I assumed that'd be the case and thought it'd be easier to get a trial PR going >.>

I'll try to stick to single-team PRs, probably easier for everyone.

[quexten]

Cross-linking the PR here: bitwarden/server#5912