Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

WIP: Cmdline changes set for 7.2 (draft)#13099

Closed
Artoria2e5 wants to merge 12 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
Artoria2e5:new-cmdlineArtoria2e5/PowerShell:new-cmdlineCopy head branch name to clipboard
Closed

WIP: Cmdline changes set for 7.2 (draft)#13099
Artoria2e5 wants to merge 12 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
Artoria2e5:new-cmdlineArtoria2e5/PowerShell:new-cmdlineCopy head branch name to clipboard

Conversation

@Artoria2e5

@Artoria2e5 Artoria2e5 commented Jul 4, 2020

Copy link
Copy Markdown

THIS IS ONLY A DRAFT. I AM HERE FOR THE CI RESULTS ON WINDOWS AND STUFF.

PR Summary

fix #1995

PR Context

See #1995, #13089, #13098.

PR Checklist

* do what PowerShell#13089 says. consolidate and fix cmdline escaping.
* an empty string check. native commands can accept empty arguments!
* attempt wildcard by spaces. now we do it by bareword.
@ghost

ghost commented Jul 4, 2020

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@Artoria2e5 Artoria2e5 marked this pull request as draft July 5, 2020 00:27
@Artoria2e5 Artoria2e5 marked this pull request as ready for review July 5, 2020 03:19
@Artoria2e5 Artoria2e5 marked this pull request as draft July 5, 2020 03:19
@Artoria2e5

Artoria2e5 commented Jul 5, 2020

Copy link
Copy Markdown
Author

How did I eff up passing a literal raw space character...


Just FYI, the build on my mac is failing with some complaints about powershellget not being located in the nuget cache. So CI is the way to go for now. Sorry for wasting all the disk space for boring logs...

@Artoria2e5

Copy link
Copy Markdown
Author

Turns out the biggest trove of code that relies on the ad hoc behavior is the pester suite. This does turn out to be a nice set of examples to illustrate the point (urgh), so that's definitely going into the writeup. We really need to add a paragraph about native stuff in about_Parameters.

Integrating this back with the legacy code to make it an experimental switch would take some time too. The code is gonna look incoherent again.

@Artoria2e5

Copy link
Copy Markdown
Author

NativeCommandArguments.Tests is gonna take some real... work.

These changes do not yet take into account the experimental feature switch needed.

After such a switch has happened, some of the tests will be changed to
explicitly specify legacy or current behavior.
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 8, 2020
@ghost ghost added the Stale label Jul 23, 2020
@ghost

ghost commented Jul 23, 2020

Copy link
Copy Markdown

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.

@ghost ghost closed this Aug 3, 2020
@Artoria2e5

Copy link
Copy Markdown
Author

Oh shit...

@ghost ghost removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept Stale labels Aug 13, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Arguments for external executables aren't correctly escaped

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.