Add tab completion for Export-Counter -FileFormat parameter #3856

MiaRomero · 2017-05-24T20:05:49Z

Fixing Export-Counter -FileFormat <tab> does not show file format options. #3180
Added ValidateSet to the -FileFormat parameter for possible log file types and updated the function that was previously validating the file formats.
Removed the invalid format error message from resources files.
Updated tests to reflect new FullyQualifiedErrorId.

TravisEz13 · 2017-05-25T01:14:54Z

src/Microsoft.PowerShell.Commands.Diagnostics/ExportCounterCommand.cs

@@ -180,9 +181,9 @@ protected override void BeginProcessing()
            _resourceMgr = Microsoft.PowerShell.Commands.Diagnostics.Common.CommonUtilities.GetResourceManager();

            //
-            // Validate the Format and CounterSamples arguments
+            // Validate output format (log file type) and CounterSamples arguments


This does not appear to be validating the format. Please correct the comment.

iSazonov · 2017-05-25T05:18:46Z

src/Microsoft.PowerShell.Commands.Diagnostics/ExportCounterCommand.cs

@@ -65,12 +65,13 @@ public string Path
                ValueFromPipelineByPropertyName = false,
                HelpMessageBaseName = "GetEventResources")]
        [ValidateNotNull]
+        [ValidateSet("blg", "csv", "tsv", IgnoreCase = true)]


We can remove IgnoreCase = true - it is by default.

adityapatwardhan · 2017-05-25T21:44:21Z

src/Microsoft.PowerShell.Commands.Diagnostics/ExportCounterCommand.cs

@@ -65,12 +65,13 @@ public string Path
                ValueFromPipelineByPropertyName = false,
                HelpMessageBaseName = "GetEventResources")]
        [ValidateNotNull]
+        [ValidateSet("blg", "csv", "tsv")]


Since we have tab completion now, should they be spelled out? CommaSeperatedValues, TabSeperatedValues and BinaryLog?

Had a chat with Jim. It is ok to keep them as is. Please open an issue on https://github.com/PowerShell/PowerShell-Docs/blob/staging/reference/5.1/Microsoft.PowerShell.Diagnostics/Export-Counter.md to explain what they mean.

The file types are explained in the 'Description' section of the documentation. Did you want them in the 'Parameter' section as well?

@MiaRomero Yes, please add them to the parameters section as well.

I've opened issue MicrosoftDocs/PowerShell-Docs#1245 for this

adityapatwardhan · 2017-05-25T21:49:00Z

test/powershell/Modules/Microsoft.PowerShell.Diagnostics/Export-Counter.Tests.ps1

@@ -180,7 +180,7 @@ Describe "Feature tests for Export-Counter cmdlet" -Tags "Feature" {
            @{


I think this test is not needed as it does not hit any code in the cmdlet. It hits Parameter Validation code paths.

Removed test case

adityapatwardhan · 2017-05-25T22:03:55Z

src/Microsoft.PowerShell.Commands.Diagnostics/ExportCounterCommand.cs

@@ -311,26 +312,21 @@ protected override void ProcessRecord()
            }
        }

-        // ValidateFormat() helper.
-        // Validates Format argument: only "BLG", "TSV" and "CSV" are valid strings (case-insensitive)
+        // SetOutputFormat() helper.


This line seems unnecessary.

Removed line 314

adityapatwardhan · 2017-05-25T22:05:57Z

src/Microsoft.PowerShell.Commands.Diagnostics/ExportCounterCommand.cs

@@ -180,9 +181,9 @@ protected override void BeginProcessing()
            _resourceMgr = Microsoft.PowerShell.Commands.Diagnostics.Common.CommonUtilities.GetResourceManager();

            //
-            // Validate the Format and CounterSamples arguments
+            // Set output format (log file type) and validate CounterSamples arguments


Please update comment, SetOuputFormat does not do any validation anymore.

Updated comment

TravisEz13 · 2017-05-26T19:32:59Z

@MiaRomero Please make your membership in the Microsoft Organization public. I sent instructions offline.

#Resolved

TravisEz13

#resolved

@adityapatwardhan

Please address @adityapatwardhan 's comments

TravisEz13 · 2017-05-31T22:57:53Z

@MiaRomero What is the status of this PR?

SteveL-MSFT

please add a test case to TabCompletion.Tests.ps1

MiaRomero · 2017-06-01T19:01:14Z

Hi @TravisEz13,
I've addressed @adityapatwardhan's and @SteveL-MSFT's comments regarding the cmdlet and test code. I added a comment asking for clarification on the documentation. Once I have that I can open the appropriate issue.

lzybkr · 2017-06-01T20:00:39Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+    It 'Should complete "Export-Counter -FileFormat" with available output formats' {
+        $res = TabExpansion2 -inputScript 'Export-Counter -FileFormat ' -cursorColumn 'Export-Counter -FileFormat '.Length
+        $res.CompletionMatches.Count | Should Be 3
+        $res.CompletionMatches.Foreach{$_.CompletionText -in 'blg', 'csv', 'tsv' | Should Be $true}


We try to avoid Should Be $true because it is difficult to understand what went wrong just reading the logs.

In this specific case, I would instead use:

$res.CompletionMatches.CompletionText -join ' ' | Should Be 'blg csv tsv'

There might be a problem with the element's order.
We can use the auxiliary output

If order is a concern, pipe to sort first. But in this case, if order is not preserved, I would call that a bug.

@lzybkr , thank you, I've made the change. Is it appropriate to go ahead and fix the other test cases in this file that use the same pattern? Or is that a separate issue/PR?

If you see the pattern in other files, then maybe a new PR, otherwise this PR is fine.

adityapatwardhan · 2017-06-01T20:01:59Z

src/Microsoft.PowerShell.Commands.Diagnostics/ExportCounterCommand.cs

@@ -65,12 +65,13 @@ public string Path
                ValueFromPipelineByPropertyName = false,
                HelpMessageBaseName = "GetEventResources")]
        [ValidateNotNull]
+        [ValidateSet("blg", "csv", "tsv")]


@MiaRomero Yes, please add them to the parameters section as well.

TravisEz13 · 2017-06-05T18:37:25Z

Please investigate test failure

TravisEz13 · 2017-06-23T02:12:16Z

I pinged @MiaRomero about updating her profile in order to get the PR merged.

MiaRomero · 2017-06-23T17:09:18Z

@TravisEz13 thanks for the link, updated my profile.

MiaRomero requested review from SteveL-MSFT and adityapatwardhan May 24, 2017 20:05

msftclas added the cla-already-signed label May 24, 2017

mirichmo assigned TravisEz13 May 25, 2017

TravisEz13 previously requested changes May 25, 2017

View reviewed changes

iSazonov reviewed May 25, 2017

View reviewed changes

MiaRomero force-pushed the export-counter-tab-completion branch from ff1a99f to 094ba3c Compare May 25, 2017 21:15

adityapatwardhan requested changes May 25, 2017

View reviewed changes

TravisEz13 reviewed May 26, 2017

View reviewed changes

SteveL-MSFT requested changes Jun 1, 2017

View reviewed changes

MiaRomero force-pushed the export-counter-tab-completion branch 2 times, most recently from e3587dc to 6633eff Compare June 1, 2017 18:16

lzybkr reviewed Jun 1, 2017

View reviewed changes

adityapatwardhan requested changes Jun 1, 2017

View reviewed changes

MiaRomero force-pushed the export-counter-tab-completion branch from 6633eff to cfe301f Compare June 2, 2017 17:40

adityapatwardhan approved these changes Jun 2, 2017

View reviewed changes

SteveL-MSFT approved these changes Jun 5, 2017

View reviewed changes

Maria Romero added 2 commits June 6, 2017 13:24

add tab completion for -FileFormat parameter

af420ac

update expected error for invalid -FileFormat test case

45a5520

MiaRomero force-pushed the export-counter-tab-completion branch from cfe301f to 45a5520 Compare June 7, 2017 00:57

TravisEz13 added Blocked blocked on something external to this repo and removed Blocked blocked on something external to this repo labels Jun 25, 2017

TravisEz13 merged commit 471f4e8 into PowerShell:master Jun 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tab completion for Export-Counter -FileFormat parameter #3856

Add tab completion for Export-Counter -FileFormat parameter #3856

		@@ -180,7 +180,7 @@ Describe "Feature tests for Export-Counter cmdlet" -Tags "Feature" {
		@{

Add tab completion for Export-Counter -FileFormat parameter #3856

Add tab completion for Export-Counter -FileFormat parameter #3856

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment