-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Change Nullable to default to enable in libraries src and ref projects #68102
Conversation
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsNow that 90% of our libraries are annotated for nulllable, it makes sense to have the default be
|
This way no CodeAnalysis attributes are emitted into the assembly.
2d13ce2
to
f2c55c6
Compare
<PropertyGroup Condition="'$(IsSourceProject)' == 'true'"> | ||
<!-- Nullability is enabled by default for all src projects. --> | ||
<PropertyGroup Condition="'$(IsSourceProject)' == 'true' or '$(IsReferenceAssemblyProject)' == 'true'"> | ||
<!-- Nullability is enabled by default for all src and ref projects. --> | ||
<Nullable Condition="'$(Nullable)' == ''">enable</Nullable> |
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.
Do we only want to set this property if it isn't yet set? Why not set it unconditionally? What if another component like the SDK or Arcade sets that property in the future and with that overrides our setting?
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.
I can't really imagine the SDK or Arcade changing the default from blank to anything other than enable
in the future. But even if they did, they still could override our setting if they were imported after this file.
In general, I prefer not unconditionally setting properties in props/targets files. If someone explicitly gets imported before this props file, and sets the value, I don't think defaulting the value should override that.
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.
I usually unconditionally overwrite properties / items if I want to set a (new) default regardless of what was previously set. In this case it sounds like we want to set our own default. That said, I don't have a strong opinion, so I'm fine leaving it as is if you prefer that.
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.
LGTM except for the few questions. Thanks Eric
runtime-dev-innerloop (Build OSX x64 release Runtime_Debug) timing out is unrelated to this change. Merging. |
Now that 90% of our libraries are annotated for nulllable, it makes sense to have the default be
enable
. There are currently 26 libraries that aren't fully annotated yet: