Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Aug 1, 2020

PR Summary

  • Autofix RCS1221

PR Context

PR Checklist


This change is Reviewable

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Aug 2, 2020

Reverted IDE0019 commits due to high number of issues.

@xtqqczze xtqqczze marked this pull request as draft August 2, 2020 17:00
@xtqqczze xtqqczze marked this pull request as ready for review August 3, 2020 19:05
@xtqqczze xtqqczze changed the title Use pattern matching instead of combination of 'as' operator and null check Fix RCS1221: Use pattern matching instead of combination of 'as' operator and null check Aug 3, 2020
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Aug 3, 2020

PR is ready for review

@rjmholt rjmholt added Review - Needed The PR is being reviewed Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers labels Aug 3, 2020
@xtqqczze
Copy link
Contributor Author

@iSazonov Can you review?

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.

LGTM with one comment.

@daxian-dbw
Copy link
Member

@PowerShell/powershell-maintainers reviewed this PR and agreed to accept it for 7.2-preivew. So we should delay merging it until after 7.1-RC1 release branch gets created.

The changes make sense, but it requires line-by-line review to make sure there is no potential incorrect change, so it's better to delay merging it till 7.2 preview to avoid possible regression.

@ghost ghost removed the Review - Needed The PR is being reviewed label Sep 1, 2020
@daxian-dbw daxian-dbw removed the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Sep 1, 2020
@iSazonov iSazonov marked this pull request as draft September 2, 2020 03:25
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 2, 2020

Set Draft till 7.2. Preview1.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 5, 2020
@ghost ghost added the Stale label Sep 20, 2020
@ghost
Copy link

ghost commented Sep 20, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@ghost ghost closed this Sep 30, 2020
@rjmholt rjmholt removed Stale Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Sep 30, 2020
@rjmholt rjmholt reopened this Sep 30, 2020
@xtqqczze
Copy link
Contributor Author

@iSazonov Please restart PowerShell-CI-windows another time.

@iSazonov
Copy link
Collaborator

Restarted CI Windows.

@ghost ghost removed the Review - Needed The PR is being reviewed label Oct 21, 2020
@xtqqczze
Copy link
Contributor Author

@iSazonov It seems Github is still reporting the old PowerShell-CI-windows result. I shall rerun tests by rebasing instead.

@xtqqczze
Copy link
Contributor Author

@iSazonov Should I fix pre-existing Codacy issues : Add curly braces around the nested statement(s) in this 'if' block.?

@iSazonov
Copy link
Collaborator

@iSazonov Should I fix pre-existing Codacy issues : Add curly braces around the nested statement(s) in this 'if' block.?

In follow PR only.

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Oct 22, 2020
@xtqqczze xtqqczze marked this pull request as draft October 26, 2020 20:17
@xtqqczze xtqqczze marked this pull request as ready for review October 29, 2020 22:10
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 5, 2020

I don't see the change in the PR but we can do this later and I am ready to merge.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Nov 5, 2020

I don't see the change in the PR but we can do this later and I am ready to merge.

I think this change was removed in a rebase to refocus the PR. Further is not changes can be made in follow-up PR. So please merge this PR.

@iSazonov iSazonov merged commit 76cef3b into PowerShell:master Nov 5, 2020
@iSazonov iSazonov assigned iSazonov and unassigned rjmholt Nov 5, 2020
@iSazonov iSazonov added this to the 7.2.0-preview.1 milestone Nov 5, 2020
@xtqqczze xtqqczze deleted the RCS1221 branch November 5, 2020 17:00
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 5, 2020

@xtqqczze Do you think about forcing Roslynator rules? We could add new GitHub Check.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Nov 5, 2020

@iSazonov I used Roslynator to make these changes as it was the easiest tool for me at the time of the PR. Now codestyle analyzers are included in .NET 5.0 we can use IDE0019 instead, see #13895.

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 5, 2020

Now codestyle analyzers are included in .NET 5.0

All?

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Nov 5, 2020

Now codestyle analyzers are included in .NET 5.0

All?

Many RCSXXXX rules have crossover with IDEXXXX and CAXXXX. If we also add StyleCop analyzers package like in #13963 we can likely cover most scenarios.

@ghost
Copy link

ghost commented Nov 17, 2020

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants