Skip to content

Enable nullable: System.Management.Automation.Language.ITypeName - #14181

Merged
Aditya Patwardhan (adityapatwardhan) merged 4 commits into
PowerShell:masterfrom
powercode:nullable/ITypeName
Apr 20, 2021
Merged

Enable nullable: System.Management.Automation.Language.ITypeName#14181
Aditya Patwardhan (adityapatwardhan) merged 4 commits into
PowerShell:masterfrom
powercode:nullable/ITypeName

Conversation

@powercode

Copy link
Copy Markdown
Collaborator

Tracking issue: #12631.

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please

  • move directives in right place
  • string AssemblyName { get; } - comment says it can be null.
  • add XML comments to non-nullable members like "This never returns null."

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 20, 2020
@iSazonov Ilya (iSazonov) added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Nov 20, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 20, 2020
@powercode

Copy link
Copy Markdown
Collaborator Author

I think we shall have a discussion before specifying nullness in the documentation.

That doesn't seem to be something that is done anywhere else, and is should anyways be automatically generated based on the type.

@iSazonov

Copy link
Copy Markdown
Collaborator

I think we shall have a discussion before specifying nullness in the documentation.

That doesn't seem to be something that is done anywhere else, and is should anyways be automatically generated based on the type.

Currently (and unlikely to change in the near future) if there is no "?" then the reader cannot understand whether the type is annotated or not. It is in these cases that I suggest adding the standard comment.

@powercode

Copy link
Copy Markdown
Collaborator Author

Sure, but the vast majority of these types are internal, and the situation is no worst than it has been before.

On the flip side, it puts an extra burden on us working with this, lowering the chance of having it done at all.

@iSazonov

Copy link
Copy Markdown
Collaborator

The work is certainly huge and at some point we will no longer remember the details of the work already done - comments will help us.
In addition, we need comments only when there is no "?", if this sign is in the signature, then everything is already obvious - this does not create a lot of additional work. We will wait longer for MSFT team approval than add comments on rare occasions. :-)

@ghost ghost added the Review - Needed The PR is being reviewed label Nov 30, 2020
@ghost

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze

Copy link
Copy Markdown
Contributor

LGTM, matches what I committed in 46fc9c97cc4b72c8b35ddb8056c2c17b2e242247 before I rebased to focus on a single interface.

@adityapatwardhan

Copy link
Copy Markdown
Member

Ilya (@iSazonov) can you update your review?

@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 16, 2021
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit b5e41e2 into PowerShell:master Apr 20, 2021
@iSazonov Ilya (iSazonov) added this to the 7.2.0-preview.6 milestone Apr 20, 2021
@powercode
Staffan Gustafsson (powercode) deleted the nullable/ITypeName branch April 27, 2021 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants