Skip to content

Implement a completion for View parameter of format cmdlets #14513

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 25 commits into from
Jul 13, 2021
Merged

Implement a completion for View parameter of format cmdlets #14513

rjmholt merged 25 commits into from
Jul 13, 2021

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Dec 30, 2020 •
edited
Loading

PR Summary

Now works dir | ft -View <Tab>.
It has always been a problem to find out which format views exist.

PR Context

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 30, 2020
@ghost ghost assigned rjmholt Dec 30, 2020
@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Dec 30, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Jan 7, 2021
@ghost
Copy link

ghost commented Jan 7, 2021

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

rjmholt
rjmholt reviewed Apr 12, 2021
View reviewed changes
@iSazonov iSazonov mentioned this pull request Apr 13, 2021
Closed
@iSazonov iSazonov removed the Documentation Needed in this repo Documentation is needed in this repo label Apr 13, 2021
@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 13, 2021
@ghost ghost added the Review - Needed The PR is being reviewed label Apr 20, 2021
@ghost
Copy link

ghost commented Apr 20, 2021

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

rjmholt
rjmholt reviewed Jun 2, 2021
View reviewed changes
Copy link
Collaborator

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

This change looks good, just have some style requests

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs Outdated
Comment on lines 5319 to 5321
Copy link
Collaborator

@rjmholt rjmholt Jun 2, 2021

Choose a reason for hiding this comment

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

What type do these vars represent?

Copy link
Collaborator Author

@iSazonov iSazonov Jun 3, 2021

Choose a reason for hiding this comment

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

Removed there var.

@iSazonov iSazonov force-pushed the completion-format-view branch from b0fbafd to 2e0c109 Compare June 3, 2021 14:34
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 3, 2021

Rebased to move to latest .Net.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 3, 2021
@rjmholt rjmholt requested a review from daxian-dbw June 10, 2021 17:31
SteveL-MSFT
SteveL-MSFT reviewed Jun 10, 2021
View reviewed changes
SteveL-MSFT
SteveL-MSFT reviewed Jun 11, 2021
View reviewed changes
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 12, 2021 

        It 'Should complete $processList = Get-Process; $processList | <cmd>' -TestCases (
            @{ cmd = 'Format-Table -View '; expected = "'R A M'", "Priority", "process", "ProcessModule", "ProcessWithUserName", "StartTime" },

I still don't understand why this test only crashed on Windows.
I directly add in the test follow:

$a="'R A M'", "Priority", "process", "ProcessModule", "ProcessWithUserName", "StartTime"
$a | sort | write-verbose

and did get:

Priority
process
ProcessModule
ProcessWithUserName
'R A M'
StartTime

On all other platform CIs (Linux and MacOS) and on my local Windows system the result was as expected:

'R A M'
Priority
process
ProcessModule
ProcessWithUserName
StartTime

I don't found why Sort-Object (and C# Array.Sort() - this I tested too) works so weird on Windows CI and add a workaround (sort expected list too).
Does anyone understand what the cause of the problem is?

@ghost ghost added the Review - Needed The PR is being reviewed label Jun 20, 2021
@ghost
Copy link

ghost commented Jun 20, 2021

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

rjmholt
rjmholt reviewed Jun 21, 2021
View reviewed changes
Copy link
Collaborator

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

The tests look good, but that quoting logic I think could be simpler

@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 21, 2021

I opened new issue in .Net Runtime to get understanding about the weird sort behavior.
Although the latter tests is more stable and we may not wait for an answer there.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 21, 2021
@iSazonov iSazonov mentioned this pull request Jun 22, 2021
Closed
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 23, 2021

dotnet/runtime#54472 (comment)

Our CI-Windows uses old Windows version without full ICU support. The link above contains a workaround if we want to have a consistency on all Windows 10 systems and use ICU instead of Windows NLS.

@iSazonov iSazonov mentioned this pull request Jun 25, 2021
Closed
@iSazonov iSazonov force-pushed the completion-format-view branch from 8d9b473 to 45ef40a Compare June 25, 2021 08:42
@ghost ghost added the Review - Needed The PR is being reviewed label Jul 8, 2021
@ghost
Copy link

ghost commented Jul 8, 2021

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

@rjmholt
Copy link
Collaborator

rjmholt commented Jul 13, 2021

@PoshChan please remind me in 4 hours

@ghost ghost removed the Review - Needed The PR is being reviewed label Jul 13, 2021
@rjmholt rjmholt enabled auto-merge (squash) July 13, 2021 17:23
@rjmholt rjmholt merged commit 0f3bc73 into PowerShell:master Jul 13, 2021
@iSazonov iSazonov deleted the completion-format-view branch July 13, 2021 17:45
@iSazonov iSazonov added this to the 7.2.0-preview.8 milestone Jul 13, 2021
@PoshChan
Copy link
Collaborator

PoshChan commented Jul 13, 2021

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

This was referenced Jul 15, 2021
Closed
Closed
@ghost
Copy link

ghost commented Jul 22, 2021

🎉v7.2.0-preview.8 has been released which incorporates this pull request.:tada:

Handy links:

@MartinGC94 MartinGC94 mentioned this pull request Jul 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

+1 more reviewer

@rjmholt rjmholt rjmholt 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.2.0-preview.8

Development

Successfully merging this pull request may close these issues.

4 participants

@iSazonov @rjmholt @PoshChan @SteveL-MSFT