Skip to content

Conversation

@powercode
Copy link
Collaborator

Tracking issue: #12631.

Should we allow null for the parameters to

WriteDebug, WriteWarning, WriteVerbose, etc?

@ghost ghost assigned rjmholt Nov 20, 2020
@rjmholt
Copy link
Collaborator

rjmholt commented Nov 20, 2020

Should we allow null for the parameters to

WriteDebug, WriteWarning, WriteVerbose, etc?

I suspect that we ideally would not, but I'm not sure if making such a change would break anything

@iSazonov
Copy link
Collaborator

IParameterMetadataProvider has 5 dependencies. This complicates understanding design intentions and we will review this later on phase 5.

@iSazonov
Copy link
Collaborator

Should we allow null for the parameters to
WriteDebug, WriteWarning, WriteVerbose, etc?

I suspect that we ideally would not, but I'm not sure if making such a change would break anything

Nullable annotations can not break anything. They only explicitly expose design intentions.

@iSazonov iSazonov marked this pull request as draft November 20, 2020 13:30
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 23, 2020
@ghost ghost added the Stale label Dec 8, 2020
@ghost
Copy link

ghost commented Dec 8, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@SteveL-MSFT SteveL-MSFT added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Dec 15, 2020
@ghost ghost closed this Dec 26, 2020
@rjmholt rjmholt reopened this Dec 28, 2020
@ghost ghost closed this Jan 7, 2021
@iSazonov iSazonov reopened this May 18, 2021
@iSazonov iSazonov removed the Stale label May 18, 2021
@iSazonov

This comment has been minimized.

@iSazonov
Copy link
Collaborator

Hmm, we never check for null in the WriteDebug() API until low level host implementations where we ignore null messages.
For WriteError() we throw if null.
For WriteObject() we accept null.
For WriteProgress() we throw if null.
For WriteVerbose we accept null (we do the same as for WriteDebug()).
For WriteWarning we accept null (we do the same as for WriteDebug()).
For WriteCommandDetail() we accept null.

@iSazonov iSazonov self-requested a review May 24, 2021 08:34
@ghost ghost added the Stale label Jun 8, 2021
@ghost
Copy link

ghost commented Jun 8, 2021

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@rjmholt
Copy link
Collaborator

rjmholt commented Jun 10, 2021

WriteObject makes sense. The throws make sense. The skips trouble me, since changing them is breaking, but it would be nice to change the type signature. But I'm not sure I see the value there. I guess the best course of action is to label the APIs as you've researched them @iSazonov

@ghost ghost removed the Stale label Jun 10, 2021
@iSazonov
Copy link
Collaborator

The skips trouble me, since changing them is breaking

Nullable annotations are only about design intentions. So we can annotate all the "skips" as non-nullable without adding real null checks and de-facto without breaking changes.
I can the commit if you want.

@rjmholt
Copy link
Collaborator

rjmholt commented Jun 10, 2021

So we can annotate all the "skips" as non-nullable without adding real null checks and de-facto without breaking changes.

Yeah that's perfect

@iSazonov
Copy link
Collaborator

The PR branch was removed so I opened new PR #15566.

@iSazonov iSazonov closed this Jun 12, 2021
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 Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants