-
Notifications
You must be signed in to change notification settings - Fork 631
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 nullability annotations #1699
base: development
Are you sure you want to change the base?
Conversation
Directory.Build.props
Outdated
@@ -4,11 +4,13 @@ | |||
<Company>fo-dicom</Company> | |||
<Copyright>Copyright © fo-dicom contributors 2012-2023</Copyright> | |||
<Description>fo-dicom</Description> | |||
<LangVersion>8</LangVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is necessary to support nullability annotations.
While .NET Standard does not officially support C# 8, everything just works. The only unsupported feature is default interface members, which we don't use. Since our unit tests are multi-targeted and include .NET Framework, we will get compilation errors there if we ever do use default interface members or something that's not allowed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #1699 +/- ##
============================================
Coverage 76.42% 76.42%
============================================
Files 275 275
Lines 25410 25413 +3
Branches 3043 3048 +5
============================================
+ Hits 19419 19423 +4
+ Misses 5062 5056 -6
- Partials 929 934 +5 ☔ View full report in Codecov by Sentry. |
I applaud this. NREs are indeed a PITA and this will mostly address them, though, as you wrote, it's a lot of work. |
I think I have the whole network layer (this is my comfort zone) covered, including its unit tests. I tried to just "describe the truth" in the annotations without making any real changes, but the annotations did uncover a few places where null might make its way through that we didn't handle yet, so in those places I added a null check or whatever was required to deal with the null value. So far, my experience has been that you do need some "domain knowledge" to annotate things properly. For example, DicomService.Association is nullable because it is indeed null in the very beginning, but after the association handshake it is always not null, so any method that is only invoked after the association handshake can safely call this.Association without concern. For the nullability annotations, this means "this.Association!" because the property could technically be null, but we know it won't be. I'm still fairly confident in some parts of the code that deal with parsing & writing DICOM to a file or a stream, but when we get into rendering territory I'm a bit more of an amateur there. So I like the idea of making issues for the different "departments" in the code, I think I will require help from you or @gofal though for the rendering parts. |
80c87cb
to
4e7e617
Compare
Same here. This is actually my only comfort zone in the code - I'm rather weak with the network part, and pratcically don't know the drawing part, as I have not been using it at all. |
@mrbean-bremen I've made a separate PR #1700 that sets up the necessary infrastructure to have nullability annotations. That PR does not contain any actual code changes, and can safely be merged. Then, it should be a lot easier to review this PR. |
I will try to review this over the weekend - won't have time before. |
No rush. This will take time anyway. 😉 |
fa3856d
to
141f956
Compare
Hi, another big modification ... fo-dicom should be considered as mature and stable. Give me some time, to get into the topic and to see, what changes are necessary for this nullable-feature. |
I agree with your assessment @gofal, and I think the risks vs benefits are very okay in this specific instance. |
10d8399
to
f7c7d39
Compare
a9253c4
to
afa5994
Compare
afa5994
to
e00be44
Compare
49a4307
to
41ed2a1
Compare
@mrbean-bremen @gofal Any chance this could get another review? Thanks in advance :-) |
ec95783
to
9ba3c38
Compare
When the amount of bytes gets large, a simple for loop starts having a measurable performance benefit, and these tests run A LOT.
9ba3c38
to
fe4b7c7
Compare
Please merge #1700 firstThis PR adds nullability annotations. Since the code base is massive, this will be a gargantuan task, so feel free to jump in and help haha.
Checklist
Changes proposed in this pull request:
Enable nullable annotations by defaultImmediately disable them again for every fileThen, enable them file by file, so we can move incrementally towards full adoption of nullability annotationsAdd inline compiler nullability attributes because .NET Standard 2.0 does not contain them, but if we add them inline it works.@gofal @mrbean-bremen I'm interested in hearing your thoughts about this. NREs have almost completely become a thing of the past in our code base at work, but here it's still a major danger. Since C# has such excellent compiler support now to avoid this, I figured I'd get things started to bring fo-dicom into a NRE-free world as well. :-)
I thought about the best approach (there really is a lot of code in fo-dicom), and enabling it globally by default and then disabling it file by file seemed like the best idea.
According to https://learn.microsoft.com/en-us/dotnet/csharp/nullable-migration-strategies, this is the best approach for libraries who are still developing new features.