Skip to content

Add unified attribute for completion for Encoding parameter. - #7732

Merged
Ilya (iSazonov) merged 3 commits into
PowerShell:masterfrom
ThreeFive-O:Fix7724
Sep 11, 2018
Merged

Add unified attribute for completion for Encoding parameter.#7732
Ilya (iSazonov) merged 3 commits into
PowerShell:masterfrom
ThreeFive-O:Fix7724

Conversation

@ThreeFive-O

@ThreeFive-O ThreeFive-O commented Sep 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

Fix #7724

Provide a tab completion attribute (ArgumentEncodingCompletion) for an Encoding parameter.
This PR should fix the duplicated code warning in CodeFactor as mentioned in issue #7724.

PR Checklist

iSazonov
Ilya (iSazonov) reviewed Sep 7, 2018
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we could add short name in TypeResolver.cs

Comment thread src/System.Management.Automation/utils/EncodingUtils.cs Outdated

@iSazonov Ilya (iSazonov) Sep 7, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the comment should describe functionality not motivation.

@ThreeFive-O ThreeFive-O Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right. I changed it.

@ThreeFive-O

ThreeFive-O commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) The ArgumentToEncodingTransformationAttribute is not in the TypeResolver.cs. We should decide if both of them are added or not.

@iSazonov

Ilya (iSazonov) commented Sep 7, 2018

Copy link
Copy Markdown
Collaborator

Sorry TypeResolver.cs is only for public types.

@iSazonov Ilya (iSazonov) self-assigned this Sep 7, 2018
@iSazonov

Ilya (iSazonov) commented Sep 7, 2018

Copy link
Copy Markdown
Collaborator

We have not received a negative feedback on encoding sets, we can consider its stable and move to one code place.

@dantraMSFT

Dan Travison (dantraMSFT) commented Sep 7, 2018

Copy link
Copy Markdown
Contributor

IMHO, the PR summary could be clearer. Something like:

Provide a tab completion attribute (ArgumentEncodingCompletion) for an Encoding parameter.

I'm not sure what the second statement means; does it or doesn't it. If so, what is it improving.
'This should improve the CodeFactor rating for the modified cmdlets.'

dantraMSFT
Dan Travison (dantraMSFT) approved these changes Sep 7, 2018
View reviewed changes

@dantraMSFT Dan Travison (dantraMSFT) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Other than rewording the Summary comment for the attribute class, this looks good.

Comment thread src/System.Management.Automation/utils/EncodingUtils.cs Outdated

@dantraMSFT Dan Travison (dantraMSFT) Sep 7, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest instead...

Provides the set of Encoding values for tab completion of an Encoding parameter.

@ThreeFive-O ThreeFive-O Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Dan Travison (@dantraMSFT) Thank you very much for this helpful suggestions. Fixed it.

@ThreeFive-O

ThreeFive-O commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

Dan Travison (@dantraMSFT) Thank you very much. I've updated the PR summary accordingly.
I hope the second part is now clearer. Also the motivation behind this PR.

@dantraMSFT

Dan Travison (dantraMSFT) commented Sep 7, 2018

Copy link
Copy Markdown
Contributor

ThreeFive-O It's very clear now. Thanks for the updates.

@iSazonov

Ilya (iSazonov) commented Sep 8, 2018

Copy link
Copy Markdown
Collaborator

ThreeFive-O Please resolve the merge conflict.

@ThreeFive-O

ThreeFive-O commented Sep 8, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) I rebased. PowerShell-CI-macos fails, but on another cmdlet:

2018-09-08T19:41:58.9111970Z Tests Passed: 78, Failed: 1, Skipped: 3, Pending: 1, Inconclusive: 0 
2018-09-08T19:42:00.2087940Z ##[debug]Reading test results from file '/Users/vsts/agent/2.140.0/work/1/s/TestResultsSudo.xml'
2018-09-08T19:42:00.2210130Z ##[debug]Processed: ##vso[results.publish type=NUnit;mergeResults=true;runTitle=Pester Sudo;publishRunAttachments=true;resultFiles=/Users/vsts/agent/2.140.0/work/1/s/TestResultsSudo.xml;]
2018-09-08T19:42:00.2233610Z TEST FAILURES
2018-09-08T19:42:00.2259100Z Description: Validate Update-Help for module 'Microsoft.PowerShell.Security' with scope as 'False'
2018-09-08T19:42:00.2287470Z Name:        Validate Update-Help from the Web for all PowerShell Core modules..Validate Update-Help for module 'Microsoft.PowerShell.Security' with scope as 'False'
2018-09-08T19:42:00.2312280Z message:
2018-09-08T19:42:00.2334570Z Expected {1}, but got {0}.
2018-09-08T19:42:00.2357580Z stack-trace:
2018-09-08T19:42:00.2380140Z at <ScriptBlock>, /Users/vsts/agent/2.140.0/work/1/s/test/powershell/engine/Help/UpdatableHelpSystem.Tests.ps1: line 192
2018-09-08T19:42:00.2406250Z 192:     $helpFilesInstalled.Count | Should -Be $expectedHelpFiles.Count
2018-09-08T19:42:00.2978340Z 1 tests in test/powershell failed
2018-09-08T19:42:00.2999130Z At /Users/vsts/agent/2.140.0/work/1/s/build.psm1:1519 char:13
2018-09-08T19:42:00.3018860Z +             throw "$($x.'test-results'.failures) tests in $TestArea f ...
2018-09-08T19:42:00.3042580Z +             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2018-09-08T19:42:00.3066190Z + CategoryInfo          : OperationStopped: (1 tests in test/powershell failed:String) [], RuntimeException
2018-09-08T19:42:00.3089720Z + FullyQualifiedErrorId : 1 tests in test/powershell failed
2018-09-08T19:42:00.3112410Z

@iSazonov

Ilya (iSazonov) commented Sep 10, 2018

Copy link
Copy Markdown
Collaborator

Reopen the PR to restart CIs.

@iSazonov

Ilya (iSazonov) commented Sep 10, 2018

Copy link
Copy Markdown
Collaborator

Travis Plunk (@TravisEz13) Again Update-Help on MacOS failed. Perhaps a bug since #6352 - I added new comment there.

I think we can merge the PR, cannot we?

@anmenaga

Andrew (anmenaga) commented Sep 10, 2018

Copy link
Copy Markdown

Restarted "PowerShell-CI-macos".
Update-help failing was a common temporary problem on several PRs.

@iSazonov
Ilya (iSazonov) merged commit 4e98f24 into PowerShell:master Sep 11, 2018
@iSazonov

Ilya (iSazonov) commented Sep 11, 2018

Copy link
Copy Markdown
Collaborator

ThreeFive-O Thanks for your contribution!

@ThreeFive-O
ThreeFive-O deleted the Fix7724 branch September 11, 2018 10:21
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@ThreeFive-O @iSazonov
Add unified attribute for completion for Encoding parameter. (PowerSh…
7e83e3e 
…ell#7732)

Provide a tab completion attribute (ArgumentEncodingCompletionsAttribute) for an Encoding parameter.
This PR should fix the duplicated code warning in CodeFactor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
+2 more reviewers
@anmenaga Andrew (anmenaga) anmenaga approved these changes
@dantraMSFT Dan Travison (dantraMSFT) dantraMSFT approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov Ilya (iSazonov)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CodeFactor: Duplicated code for argument completions attribute class

5 participants

@ThreeFive-O @iSazonov @dantraMSFT @anmenaga @daxian-dbw