Add 'Nullable' option to the build property page#5807
Add 'Nullable' option to the build property page#5807tmeschter merged 3 commits intodotnet:masterfrom
Conversation
This option is only available when the language version is 8.0 or greater.
|
I think we should consider coming up with a way for this property to visible but disabled when changing the TFM will cause it to be enabled.
|
davidwengier
left a comment
There was a problem hiding this comment.
Is it worth having a link to an article explaining what the various values mean? If we do the "visible but disabled" thing I think a link explaining why its disabled would be vital.
|
This is the help for that page: https://docs.microsoft.com/en-us/visualstudio/ide/reference/build-page-project-designer-csharp?f1url=https%3A%2F%2Fmsdn.microsoft.com%2Fquery%2Fdev16.query%3FappId%3DDev16IDEF1%26l%3DEN-US%26k%3Dk(cs.ProjectPropertiesBuild)%26rd%3Dtrue&view=vs-2019. We should document it there. |
|
Changing it to disable rather than hide is straightforward to do, but I felt it would make the UI more confusing for users. Currently the state of this UI is not updated based on changes to TFM (via either UI or file on disk). The page needs to be reloaded. I looked at We could justify sticking help links on every UI element on the page. Does this one deserve one just because it's new? Once the UI here is finalised I'll update the docs. |
|
We don't hide elements in other cases where changing the TFM would enable it - for example, Prefer 32-bit. Can you see how it reacts when you change from 4.0 -> 4.5 in legacy? |
I think this one deserves something because "Enable", "Warnings" and "Annotations" don't mean much on their own. |
|
Updated to respond to changes to Also changed to disable rather than hide the UI. |
I think you could argue this about any of the options you weren't familiar with. For example, what does the The property pages support F1 help, though it's not very discoverable and is currently broken (I filed #5815). |
|
Documentation updated in https://github.com/MicrosoftDocs/visualstudio-docs-pr/pull/6024. |
tmeschter
left a comment
There was a problem hiding this comment.
By putting this on the Build properties page you're making this a configuration- and platform-specific setting. Nullable doesn't seem like the kind of setting that you want to behave differently in debug and release builds, for example.
If you want it to be configuration-independent, the best option is the Advanced Build Options dialog.
| </data> | ||
| <data name="PPG_BuildSettings_Nullable_Annotations" xml:space="preserve"> | ||
| <value>Annotations</value> | ||
| </data> |
There was a problem hiding this comment.
By placing these values in .resx file we're choosing to localize them. Is that what you want, or would it make more sense to simply use the literal values understood by the compiler?
In the case of the latter you would hard-code them in the source and use the lower-case forms.
There was a problem hiding this comment.
The drop down should be localized, what we persist in the project file shouldn't be.
There was a problem hiding this comment.
I considered using the compiler's strings and ended up following the example set by the advanced build properties page (where I initially added this new UI before moving it to the regular build properties page).
There was a problem hiding this comment.
Yes, and these were localized in response to a bug.
|
The "Advanced Build Options" page is also configuration-dependent. I think you can argue this for lots of options on that page, unsafe code, Prefer 32-bit, etc. |
To be clear, in this PR the setting behaves in a configuration-independent manner due to The question is whether this behaviour is surprising to the user or not. @davkean @davidwengier and I discussed placement on Monday and decided the setting was not really 'advanced', and that it deserved a more prominent position in the UI. So I moved it from the advanced build prop page to the regular build prop page. But looking at it now I see that every other property on that page is actually configuration-dependent. I agree that nullable shouldn't be configuration-dependent. It's a shame to hide the option further down, but on balance I think you're right so will move it back 🤦♂ |
|
Actually it turns out that every setting on the advanced property page is configuration-dependent too. So now I'm less sure that moving it does anything useful. When language version was editable, it would have been the exception on the two build pages. We could move it to the application page, but it's not clear it belongs there either. |
|
Sorry, I missed |
Fixes #4058.
This option is only available when the language version is 8.0 or greater.
netcoreapp3.1 (LangVersion 8)
The dropdown has the following options:
net48 (LangVersion 7.3)
The nullable control is disabled: