Skip to content

Use is not syntax and remove unnecessary parentheses #13323

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
daxian-dbw merged 6 commits into from
Jul 31, 2020
Merged

Use is not syntax and remove unnecessary parentheses #13323

daxian-dbw merged 6 commits into from
Jul 31, 2020

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 30, 2020

PR Summary

Instead of the huge changes in #13286, a more modest change using is not syntax to remove parentheses.

PR Context

#13286

PR Checklist

@xtqqczze
Use is not syntax
bf0f1ea 
-replace '\(!\(([^)]+) is ([^)]+)\)\)', '($1 is not $2)'
@ghost ghost assigned daxian-dbw Jul 30, 2020
@xtqqczze xtqqczze marked this pull request as ready for review July 30, 2020 23:41
@xtqqczze xtqqczze mentioned this pull request Jul 30, 2020
Closed
14 tasks
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 31, 2020

@daxian-dbw Can you restart CI?

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 31, 2020

It appears Codacy (SonarCSharp) does not understand C# 9 pattern matching.

@rjmholt
Copy link
Collaborator

rjmholt commented Jul 31, 2020

/azp help

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 31, 2020

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@rjmholt
Copy link
Collaborator

rjmholt commented Jul 31, 2020

/azp list

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 31, 2020

CI/CD Pipelines for this repository:

@rjmholt
Copy link
Collaborator

rjmholt commented Jul 31, 2020

It looks like the same test is failing across all platforms, implying a regression. In that case, I think it's safer for us not to take this change

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 31, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 31, 2020

Commenter does not have sufficient privileges for PR 13323 in repo PowerShell/PowerShell

vexx32
vexx32 suggested changes Jul 31, 2020
View reviewed changes
Copy link
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Found a couple places where the substitution mangled the original logic. Once we get those fixed we should be right to go. 🙂

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 31, 2020
@xtqqczze @vexx32
Apply suggestions from code review
bd1f4c4 
Co-authored-by: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 31, 2020
@rjmholt rjmholt added Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers Review - Needed The PR is being reviewed labels Jul 31, 2020
xtqqczze and others added 2 commits July 31, 2020 16:35
vexx32
vexx32 approved these changes Jul 31, 2020
View reviewed changes
Copy link
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for sorting this out! 💖

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 31, 2020

I have fully reviewed this PR and it is now ready to merge as far as I am concerned.

daxian-dbw
daxian-dbw approved these changes Jul 31, 2020
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@daxian-dbw
Copy link
Member

daxian-dbw commented Jul 31, 2020

@PoshChan Please remind me in 1 hour

@ghost ghost removed the Review - Needed The PR is being reviewed label Jul 31, 2020
@PoshChan
Copy link
Collaborator

PoshChan commented Jul 31, 2020

@daxian-dbw, this is the reminder you requested 1 hour ago

@daxian-dbw daxian-dbw merged commit 4597b41 into PowerShell:master Jul 31, 2020
@daxian-dbw daxian-dbw added CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log and removed Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers labels Jul 31, 2020
@daxian-dbw daxian-dbw added this to the 7.1.0-preview.7 milestone Jul 31, 2020
@xtqqczze xtqqczze deleted the isnot branch August 1, 2020 16:59
@xtqqczze xtqqczze mentioned this pull request Aug 1, 2020
Merged
8 tasks
@baosj0
Copy link

baosj0 commented Aug 2, 2020

this "is not "syntax make my build failed using visualstudio2019

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Aug 2, 2020 •
edited
Loading

@baosj0 You need to use Visual Studio 2019 16.7 Preview (link), I have opened a PR to update build documentation (#13336).

This was referenced Aug 2, 2020
Merged
Merged
@baosj0
Copy link

baosj0 commented Aug 3, 2020

@baosj0 You need to use Visual Studio 2019 16.7 Preview (link), I have opened a PR to update build documentation (#13336).

yes , no problem now

@TravisEz13 TravisEz13 modified the milestones: 7.1.0-preview.7, 7.1.0-preview.6 Aug 5, 2020
@ghost
Copy link

ghost commented Aug 17, 2020

🎉v7.1.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

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

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga

+1 more reviewer

@vexx32 vexx32 vexx32 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw daxian-dbw

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.6

Development

Successfully merging this pull request may close these issues.

7 participants

@xtqqczze @rjmholt @daxian-dbw @PoshChan @baosj0 @vexx32 @TravisEz13