Skip to content

Make the parameter 'args' no-nullable in the public ConsoleHost APIs #13429

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.

Sign up for GitHub

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

Jump to bottom
Merged
rjmholt merged 3 commits into from
Aug 14, 2020
Merged

Make the parameter 'args' no-nullable in the public ConsoleHost APIs #13429

rjmholt merged 3 commits into from
Aug 14, 2020

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Aug 13, 2020 •
edited
Loading

PR Summary

This is a follow-up on #11482
We actually don't expect args to contain null value in it. For example, the use of args in GetSwitchKey at here indicates we are not expecting an element of args is null, and many other places like ParseFile.

/cc @iSazonov Maybe we should validate if args contains any null elements, maybe that's not necessary given it has been working fine so far, but either way, I'm deferring that to a separate PR.

PR Context

PR Checklist

@daxian-dbw daxian-dbw requested a review from anmenaga as a code owner August 13, 2020 21:35
@ghost ghost assigned iSazonov Aug 13, 2020
@daxian-dbw daxian-dbw assigned rjmholt and unassigned iSazonov Aug 13, 2020
@rjmholt
Copy link
Collaborator

rjmholt commented Aug 13, 2020

@PoshChan please remind me in 24 hours

@daxian-dbw daxian-dbw requested a review from TravisEz13 as a code owner August 13, 2020 23:02
rjmholt
rjmholt reviewed Aug 13, 2020
View reviewed changes
@rjmholt rjmholt merged commit d9c2e0e into PowerShell:master Aug 14, 2020
@daxian-dbw daxian-dbw deleted the consoleAPIs branch August 14, 2020 00:07
@iSazonov
Copy link
Collaborator

iSazonov commented Aug 14, 2020

Maybe we should validate if args contains any null elements, maybe that's not necessary given it has been working fine so far, but either way, I'm deferring that to a separate PR.

.Net team confirmed that it is not null for Main() - right signature Main(string[] args).
For UnmanagedEntry users could send null. We do not check this previously so we can (1) document this explicitly that it is Main() signature, (2) add the check.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Aug 14, 2020

For UnmanagedEntry users could send null.

Why is that? Yes, there is a null assignment in Program.cs but that's the arg for exec, which requires a null element to indicate the end of args. I don't think that null will be passed to UnmanagedEntry.Start.

ConsoleShell.Start is public API, so a user potentially can pass in a string array with null elements. If we want to validate args, then I guess that should be done in CommandLineParameterParser.Parse.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 14, 2020

I don't think that null will be passed to UnmanagedEntry.Start

It is a public API too :-) It would be not right using but possible.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 14, 2020

I guess that should be done in CommandLineParameterParser.Parse.

What is desired behavior for null? Throw?

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Aug 14, 2020

It is a public API too :-) It would be not right using but possible.

True, but it's less likely to be used like the ConsoleShell.Start.

What is desired behavior for null? Throw?

Yes, throw, but with ArgumentException and a better error message.

@rjmholt rjmholt added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 14, 2020
@rjmholt rjmholt added this to the 7.1.0-preview.6 milestone Aug 14, 2020
@PoshChan
Copy link
Collaborator

PoshChan commented Aug 14, 2020

@rjmholt, this is the reminder you requested 24 hours ago

@iSazonov iSazonov mentioned this pull request Aug 15, 2020
Merged
14 tasks
@ghost
Copy link

ghost commented Aug 17, 2020

🎉v7.1.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@iSazonov iSazonov Awaiting requested review from iSazonov

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

+2 more reviewers

@rjmholt rjmholt rjmholt approved these changes

@anmenaga anmenaga anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rjmholt rjmholt

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.6

Development

Successfully merging this pull request may close these issues.

6 participants

@daxian-dbw @rjmholt @iSazonov @PoshChan @anmenaga @adityapatwardhan