Skip to content

Add unified attribute for completion for Encoding parameter. #7732

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
iSazonov merged 3 commits into from
Sep 11, 2018
Merged

Add unified attribute for completion for Encoding parameter. #7732

iSazonov merged 3 commits into from
Sep 11, 2018

Conversation

@ThreeFive-O
Copy link
Contributor

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

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
iSazonov reviewed Sep 7, 2018
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

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

src/System.Management.Automation/utils/EncodingUtils.cs Outdated
Copy link
Collaborator

@iSazonov iSazonov Sep 7, 2018

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.

Copy link
Contributor Author

@ThreeFive-O ThreeFive-O Sep 7, 2018

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
Copy link
Contributor Author

ThreeFive-O commented Sep 7, 2018

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

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 7, 2018

Sorry TypeResolver.cs is only for public types.

@iSazonov iSazonov self-assigned this Sep 7, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 7, 2018

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

@dantraMSFT
Copy link
Contributor

dantraMSFT commented Sep 7, 2018

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
dantraMSFT approved these changes Sep 7, 2018
View reviewed changes
Copy link
Contributor

@dantraMSFT dantraMSFT left a comment

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.

src/System.Management.Automation/utils/EncodingUtils.cs Outdated
Copy link
Contributor

@dantraMSFT dantraMSFT Sep 7, 2018

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.

Copy link
Contributor Author

@ThreeFive-O ThreeFive-O Sep 7, 2018

Choose a reason for hiding this comment

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

@dantraMSFT Thank you very much for this helpful suggestions. Fixed it.

@ThreeFive-O
Copy link
Contributor Author

ThreeFive-O commented Sep 7, 2018

@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
Copy link
Contributor

dantraMSFT commented Sep 7, 2018

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

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 8, 2018

@ThreeFive-O Please resolve the merge conflict.

@ThreeFive-O
Copy link
Contributor Author

ThreeFive-O commented Sep 8, 2018

@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 iSazonov closed this Sep 10, 2018
@iSazonov iSazonov reopened this Sep 10, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 10, 2018

Reopen the PR to restart CIs.

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 10, 2018

@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
Copy link

anmenaga commented Sep 10, 2018

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

@iSazonov iSazonov merged commit 4e98f24 into PowerShell:master Sep 11, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 11, 2018

@ThreeFive-O Thanks for your contribution!

@ThreeFive-O ThreeFive-O deleted the Fix7724 branch September 11, 2018 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@iSazonov iSazonov iSazonov approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

+2 more reviewers

@anmenaga anmenaga anmenaga approved these changes

@dantraMSFT dantraMSFT dantraMSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov 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