8000 Code redundancy fixes pass 2 by xtqqczze · Pull Request #13334 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Code redundancy fixes pass 2 #13334

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
Closed
xtqqczze wants to merge 0 commits into from
Closed

Code redundancy fixes pass 2 #13334

xtqqczze wants to merge 0 commits into from

Conversation

@xtqqczze
Copy link
Contributor
@xtqqczze xtqqczze commented Aug 1, 2020 •
edited
Loading

All changes have been merged.

Follow-up to #12916

@ghost ghost assigned TravisEz13 Aug 1, 2020
@xtqqczze xtqqczze force-pushed the RCS-Redundancy-fixes2 branch from 382d65a to 77141a4 Compare August 2, 2020 17:29
@xtqqczze
Copy link
Contributor Author
xtqqczze commented Aug 2, 2020

rebased to restart CI

iSazonov
iSazonov reviewed Aug 2, 2020
View reviewed changes
@daxian-dbw daxian-dbw added the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Aug 6, 2020
@TravisEz13
Copy link
Member
TravisEz13 commented Aug 11, 2020

Marked for maintainer review because of large line changes.

TravisEz13
TravisEz13 reviewed Aug 11, 2020
View reviewed changes
Copy link
Member
@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

I'm not going to say to change anything, because I think that this PR is not viable.

xtqqczze added a commit to xtqqczze/PowerShell-PowerShell that referenced this pull request Aug 11, 2020
@xtqqczze
Autofix RCS1036: Remove redundant empty line
e965934 
Fixes SA1506: A documentation header line must not be followed by a blank line.

Follow-up to PowerShell#13334, PowerShell#13334
@xtqqczze xtqqczze mentioned this pull request Aug 11, 2020
Merged
8 tasks
@xtqqczze xtqqczze force-pushed the RCS-Redundancy-fixes2 branch from 77141a4 to 02f4b96 Compare August 11, 2020 22:29
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 14, 2020
@TravisEz13
Copy link
Member
TravisEz13 commented Aug 14, 2020 •
edited
Loading

I'm leaving the review-maintainer tag, so the maintainers can review what was done here and see if we should update the contribution guide.

One suggestion, is that guidance should be:

  • limit PR size
  • Fix One issue per PR

Although it is guidance, the guidance is a consideration during PR review.

@iSazonov
Copy link
Collaborator
iSazonov commented Aug 14, 2020

limit PR size

It seems we already have this recommendation. Also for automatic fixes (by script or analyzer) it is better to fix all code base at a time.

@TravisEz13
Copy link
Member
TravisEz13 commented Aug 14, 2020

It seems we already have this recommendation. Also, for automatic fixes (by script or analyzer) it is better to fix all code base at a time.

Then, I'm at a deadlock. Large fixes are not reasonably reviewable.

@xtqqczze
Copy link
Contributor Author
xtqqczze commented Aug 14, 2020 •
edited
Loading

OK, I will seperate RCS1114: Remove redundant delegate creation changes into new PR, so that they can be reviewed and merged. I will not rebase this PR, leaving @iSazonov approval intact. This means we will have to merge in order.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 14, 2020
@xtqqczze xtqqczze mentioned this pull request Aug 14, 2020
Merged
8 tasks
@iSazonov
Copy link
Collaborator
iSazonov commented Aug 15, 2020

Then, I'm at a deadlock. Large fixes are not reasonably reviewable.

We have already gained some experience in the area:

  • do only one kind of a change per PR
  • combine changes in some small commits and review by commit.
  • pull PRs per module/project, for SMA - per subsystem.

@xtqqczze xtqqczze mentioned this pull request Aug 15, 2020
Merged
9 tasks
@xtqqczze
Copy link
Contributor Author
xtqqczze commented Aug 15, 2020

@iSazonov @TravisEz13 I have split some changes from RCS1033: Remove redundant boolean literal to #13454 based upon project.

@daxian-dbw
Copy link
Member
daxian-dbw commented Sep 1, 2020

@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 and the chance of confliction is relatively low, 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.

@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:23
@iSazonov
Copy link
Collaborator
iSazonov commented Sep 2, 2020 •
edited
Loading

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
@iSazonov iSazonov reopened this Oct 16, 2020
@iSazonov
Copy link
Collaborator
iSazonov commented Oct 16, 2020

@xtqqczze Is it still Draft?

@ghost ghost removed the Stale label Oct 16, 2020
@xtqqczze xtqqczze force-pushed the RCS-Redundancy-fixes2 branch from 02f4b96 to 0f64aca Compare October 22, 2020 22:51
@xtqqczze
Copy link
Contributor Author
xtqqczze commented Oct 22, 2020

rebased

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 22, 2020
@xtqqczze xtqqczze marked this pull request as ready for review October 22, 2020 22:51
@xtqqczze

This comment has been minimized.

Sign in to view
@xtqqczze
Copy link
Contributor Author
xtqqczze commented Oct 22, 2020

remaining CodeFactor issues are pre-existing

@ghost ghost added the Review - Needed The PR is being reviewed label Oct 30, 2020
@ghost
Copy link
ghost commented Oct 30, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze xtqqczze mentioned this pull request Nov 2, 2020
Closed
@iSazonov
Copy link
Collaborator
iSazonov commented Nov 5, 2020

@xtqqczze Please resolve merge conflicts.

@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 5, 2020
@iSazonov
Copy link
Collaborator
iSazonov commented Nov 6, 2020

@TravisEz13 Please update your review.

@xtqqczze
Copy link
Contributor Author
xtqqczze commented Nov 6, 2020

@iSazonov I'm minded to close this in favour of #13994, which should include all of these changes. This PR has been open for many months, I think a fresh start would be easier to review.

@xtqqczze xtqqczze closed this Nov 13, 2020
@xtqqczze xtqqczze force-pushed the RCS-Redundancy-fixes2 branch from 7976046 to b521b85 Compare November 13, 2020 05:37
@xtqqczze
Copy link
Contributor Author
xtqqczze commented Nov 13, 2020

all changes are merged, closed

@xtqqczze xtqqczze deleted the RCS-Redundancy-fixes2 branch November 13, 2020 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@TravisEz13 TravisEz13 TravisEz13 requested changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

Assignees

@TravisEz13 TravisEz13

Labels

size/XL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xtqqczze @TravisEz13 @iSazonov @daxian-dbw
0