Fix nullable references #168
Conversation
cs files updated for nullable enable
|
@twsouthwick, please review and make sure everything looks OK. |
|
|
||
| private static void CreatePresentationParts(PresentationPart presentationPart) |
There was a problem hiding this comment.
why is private removed?
There was a problem hiding this comment.
Because with private set I get "CS0106: The modifier 'private' is not valid for this item" error from Visual Studio.
samples/presentation/retrieve_the_number_of_slides/cs/Program.cs
Outdated
Show resolved
Hide resolved
| wordDoc.MainDocumentPart.WordprocessingCommentsPart; | ||
| if (wordDoc.MainDocumentPart is null || wordDoc.MainDocumentPart.WordprocessingCommentsPart is null) | ||
| { | ||
| throw new System.NullReferenceException("MainDocumentPart and/or WordprocessingCommentsPart is null."); |
There was a problem hiding this comment.
instead of nullreferenceexception, should throw InvalidOperation
|
Learn Build status updates of commit 06a0984: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
@mikeebowen I think the only blocking thing here is to remove the manual throwing of the NullReferenceExceptions. It would be nice to add |
|
Do you want to change all the NullReferenceExceptions to ArgumentNullException? Many of them are not null arguments, but possibly null parts and elements of the document. |
It is a big code smell to throw NullReferenceExceptions - you are supposed to throw ArgumentNullExceptions if you have something that should not be null but is. If you don't think that's appropriate, then you'll want to refactor things so that you don't need to throw either. |
|
@twsouthwick Please ping me when all comments are resolved and you're ready to merge this PR! Thanks, Linda |
|
Learn Build status updates of commit cac8345: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit c9a56a3: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit dd2e343: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
There was a problem hiding this comment.
LGTM thanks for responding to the feedback!
@lindalu-MSFT this can be merged now
|
@lindalu-MSFT is there a staging build we can use to see the final output as we work through the items in #159 ? |
I don't know about .cs code, but for docs, you go to the live location you want to review the changes and insert "review." at the front of the URL. For example, https://learn.microsoft.com/en-us/office/open-xml/open-xml-sdk becomes https://review.learn.microsoft.com/en-us/office/open-xml/open-xml-sdk. |
Perfect! The code gets pulled in via includes, so they'll show up there and we can validate things actually look right (we can only get so far with the docfx cli) |
|
@twsouthwick Please resolve these merge conflicts then I will merge to live. |
This pull request fixes nullable reference errors in the samples and makes them executable with command line arguments.