8000 Add Culture parameter to Select-String cmdlet by iSazonov · Pull Request #10943 · PowerShell/PowerShell · GitHub 8000
[go: up one dir, main page]
Skip to content

Add Culture parameter to Select-String cmdlet #10943

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
adityapatwardhan merged 6 commits into from
Nov 16, 2019
Merged

Add Culture parameter to Select-String cmdlet #10943

adityapatwardhan merged 6 commits into from
Nov 16, 2019

Conversation

@iSazonov
Copy link
Collaborator
@iSazonov iSazonov commented Oct 30, 2019 •
edited
Loading

PR Summary

Fix #8180
Add new Culture parameter to Select-String cmdlet.

PR Context

Implement PowerShell/PowerShell-RFC#167 for Select-String cmdlet.

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 Oct 30, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.6 milestone Oct 30, 2019
@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Oct 30, 2019
SteveL-MSFT
SteveL-MSFT requested changes Oct 30, 2019
View reviewed changes
@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Oct 30, 2019
SteveL-MSFT
SteveL-MSFT approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member
@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

Please address Codacy issues

@iSazonov
Copy link
Collaborator Author
iSazonov commented Nov 1, 2019

Please address Codacy issues

I fix one but second impossible (interface implementation).

@iSazonov iSazonov force-pushed the add-culture-selectstring branch from 30524de to aa9cc09 Compare November 1, 2019 14:05
PaulHigin
PaulHigin suggested changes Nov 4, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs
StringComparison compareOption = CaseSensitive ?
StringComparison.CurrentCulture : StringComparison.CurrentCultureIgnoreCase;
while (patternIndex < Pattern.Length)
{
Copy link
Contributor
@PaulHigin PaulHigin Nov 4, 2019

Choose a reason for hiding this comment

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

This old 'StringComparison compareOption = ...' code can be removed.

Copy link
Collaborator Author
@iSazonov iSazonov Nov 13, 2019

Choose a reason for hiding this comment

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

Done.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 4, 2019
@adityapatwardhan
Copy link
Member
adityapatwardhan commented Nov 12, 2019

@iSazonov Can you respond to comment from @PaulHigin

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 13, 2019
@iSazonov iSazonov mentioned this pull request Nov 13, 2019
Closed
10 tasks
@iSazonov iSazonov removed the Documentation Needed in this repo Documentation is needed in this repo label Nov 13, 2019
@iSazonov
Copy link
Collaborator Author
iSazonov commented Nov 13, 2019

@adityapatwardhan Paul's comment addressed. Doc issue was filled.

TravisEz13
TravisEz13 reviewed Nov 14, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs
/// <summary>
/// Get list of valid culture names for ValidateSet attribute.
/// </summary>
public class ValidateMatchStringCultureNamesGenerator : IValidateSetValuesGenerator
Copy link
Member
@TravisEz13 TravisEz13 Nov 14, 2019

Choose a reason for hiding this comment

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

Can this be internal?

Copy link
Collaborator Author
@iSazonov iSazonov Nov 15, 2019

Choose a reason for hiding this comment

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

It must be public:

PowerShell/src/System.Management.Automation/engine/Attributes.cs

Lines 1715 to 1723 in fe40fbf

public ValidateSetAttribute(Type valuesGeneratorType)
{
// We check 'IsNotPublic' because we don't want allow 'Activator.CreateInstance' create an
// instance of non-public type.
if (!typeof(IValidateSetValuesGenerator).IsAssignableFrom(
valuesGeneratorType) || valuesGeneratorType.IsNotPublic)
{
throw PSTraceSource.NewArgumentException(nameof(valuesGeneratorType));
}

@adityapatwardhan
Copy link
Member
adityapatwardhan commented Nov 15, 2019

@iSazonov Please respond the comment from @TravisEz13

@adityapatwardhan adityapatwardhan merged commit ff37337 into PowerShell:master Nov 16, 2019
@iSazonov iSazonov deleted the add-culture-selectstring branch November 16, 2019 09:22
@ghost
Copy link
ghost commented Nov 21, 2019

🎉v7.0.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 left review comments

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

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

+2 more reviewers

@PoshChan PoshChan PoshChan left review comments

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.0.0-preview.6

Development

Successfully merging this pull request may close these issues.

Select-String -SimpleMatch does only current culture search

6 participants

@iSazonov @adityapatwardhan @TravisEz13 @SteveL-MSFT @PaulHigin @PoshChan
0