GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add tab completion for Export-Counter -FileFormat parameter by MiaRomero · Pull Request #3856 · PowerShell/PowerShell

Conversation

Maria Romero (MiaRomero) commented May 24, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) requested review from Steve Lee (SteveL-MSFT) and Aditya Patwardhan (adityapatwardhan)

May 24, 2017 20:05

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label

May 24, 2017

Mike Richmond (mirichmo) assigned Travis Plunk (TravisEz13)

May 25, 2017

Travis Plunk (TravisEz13) previously requested changes

May 25, 2017

View reviewed changes

Comment thread

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

Travis Plunk (TravisEz13) May 25, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) May 25, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) reviewed

May 25, 2017

View reviewed changes

Comment thread

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

Ilya (iSazonov) May 25, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) May 25, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) force-pushed the export-counter-tab-completion branch from ff1a99f to 094ba3c Compare

May 25, 2017 21:15

Aditya Patwardhan (adityapatwardhan) requested changes

May 25, 2017

View reviewed changes

Comment thread

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

Aditya Patwardhan (adityapatwardhan) May 25, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) May 26, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) Jun 1, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) Jun 1, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maria Romero (@MiaRomero) Yes, please add them to the parameters section as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) Jun 2, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Aditya Patwardhan (adityapatwardhan) May 25, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) Jun 1, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed test case

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Aditya Patwardhan (adityapatwardhan) May 25, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This line seems unnecessary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) Jun 1, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed line 314

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Aditya Patwardhan (adityapatwardhan) May 25, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) Jun 1, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Updated comment

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) commented May 26, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

#Resolved

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) reviewed

May 26, 2017

View reviewed changes

Travis Plunk (TravisEz13) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

#resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) dismissed their stale review

May 26, 2017 20:54

Please address @adityapatwardhan 's comments

Travis Plunk (TravisEz13) commented May 31, 2017

Copy link

Copy Markdown

Member

Maria Romero (@MiaRomero) What is the status of this PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) requested changes

Jun 1, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

please add a test case to TabCompletion.Tests.ps1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) force-pushed the export-counter-tab-completion branch 2 times, most recently from e3587dc to 6633eff Compare

June 1, 2017 18:16

Maria Romero (MiaRomero) commented Jun 1, 2017

Copy link

Copy Markdown

Member Author

Hi Travis Plunk (@TravisEz13),
I've addressed Aditya Patwardhan (@adityapatwardhan)'s and Steve Lee (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jason Shirk (lzybkr) reviewed

Jun 1, 2017

View reviewed changes

Comment thread

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

Jason Shirk (lzybkr) Jun 1, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adityapatwardhan reacted with thumbs up emoji

All reactions

👍 1 reaction

Ilya (iSazonov) Jun 2, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Jun 2, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction

Maria Romero (MiaRomero) Jun 2, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Jason Shirk (@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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Jun 2, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) requested changes

Jun 1, 2017

View reviewed changes

Comment thread

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

Aditya Patwardhan (adityapatwardhan) Jun 1, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maria Romero (@MiaRomero) Yes, please add them to the parameters section as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Maria Romero (MiaRomero) force-pushed the export-counter-tab-completion branch from 6633eff to cfe301f Compare

June 2, 2017 17:40

Aditya Patwardhan (adityapatwardhan) approved these changes

Jun 2, 2017

View reviewed changes

Travis Plunk (TravisEz13) commented Jun 5, 2017

Copy link

Copy Markdown

Member

Please investigate test failure

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (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

Maria Romero (MiaRomero) force-pushed the export-counter-tab-completion branch from cfe301f to 45a5520 Compare

June 7, 2017 00:57

Travis Plunk (TravisEz13) commented Jun 23, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Maria Romero (MiaRomero) commented Jun 23, 2017

Copy link

Copy Markdown

Member Author

Travis Plunk (@TravisEz13) thanks for the link, updated my profile.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) added Blocked blocked on something external to this repo and removed Blocked blocked on something external to this repo labels

Jun 25, 2017

Travis Plunk (TravisEz13) merged commit 471f4e8 into PowerShell:master

Jun 25, 2017

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Add tab completion for Export-Counter -FileFormat parameter (PowerShe…

3900a75

…ll#3856)

* add tab completion for -FileFormat parameter

* update expected error for invalid -FileFormat test case

Reviewers

Travis Plunk (TravisEz13)

TravisEz13 left review comments

Ilya (iSazonov)

iSazonov left review comments

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

+1 more reviewer

Jason Shirk (lzybkr)

lzybkr left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Maria Romero (MiaRomero) commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Travis Plunk (TravisEz13) commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Travis Plunk (TravisEz13) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Travis Plunk (TravisEz13) commented May 31, 2017

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Maria Romero (MiaRomero) commented Jun 1, 2017

Uh oh!

Jason Shirk (lzybkr) Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Travis Plunk (TravisEz13) commented Jun 5, 2017

Uh oh!

Travis Plunk (TravisEz13) commented Jun 23, 2017

Uh oh!

Maria Romero (MiaRomero) commented Jun 23, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Maria Romero (MiaRomero) commented May 24, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) commented May 26, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Jason Shirk (lzybkr) Jun 1, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.