Sort and Merge Classes in WebCmdlets\Common #5662

markekraus · 2017-12-09T14:31:38Z

PR Summary

Reference #5610

Merges partial classes in the WebCmdlets\Common folder
Sorts the using statements
Sorts class elements according to StyleCop guidance https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1201.md
Sorts within the same elements by access type according to StyleCop guidance https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1202.md
Sorts elements of the same type and access alphabetically.
Order of cmdlet public properties and parameters was untouched to preserve their order in PowerShell
Proper inheritance was added to InvokeRestMethodCommand.
Some new regions were created where it makes sense
Formatting and comments of code blocks was purposefully maintained. These will be handled in a separate cleanup action.
No logic was modified

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
NA User facing Documentation needed
- NA Issue filed - Issue link:
Change is not breaking
NA Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affectes feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

iSazonov · 2017-12-13T13:02:47Z

...rosoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/ContentHelper.Common.cs

+            return GetEncodingOrDefault(charSet);
        }

        internal static Encoding GetEncodingOrDefault(string characterSet)


@markekraus We could consider to change the default to Utf8 in later PR.

iSazonov · 2017-12-13T13:16:49Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

-using System.Collections.Generic;
-using System.Text.RegularExpressions;
-using System.Linq;
+using Microsoft.Win32;


Do we really need this?

It looks like we don't. Should I remove it in this PR or in another?

Better in next - the PR is very large :-)

I have added it to the task list in #5610

iSazonov · 2017-12-13T13:27:09Z

...ft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebResponseObject.Common.cs

 --********************************************************************/

 using System;
+using System.Collections.Generic;


The file is not sorted but I agree to merge "as is" - if we'll have follow formatting PR.

@iSazonov I'm confused. This file is sorted. What is out of order?

ah. that one can probably be fixed with automatic property { get; private set }, which is on the checklist already.

@markekraus Pow("Please", 10) make the follow PRs as small as possible - split work by file/type formattings. :-)

@iSazonov if you have ideas on how the work should be split, please update #5610. I still don't have a good handle on what is too big or too small and, honestly, sometimes the feedback on that is inconsistent.

Maybe interesting https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/

TravisEz13 · 2017-12-19T19:56:47Z

If you have enabled styleCop, can you enable https://www.codefactor.io on your branch so I can see that your changes improve the style?
Such a large amount of code was changed, I have to take your word that no logic was changed. Can we break this up into smaller PRs to get this done? The following are ideas on how to break it up:
- Perhaps merge the partial classes without sorting ( should be easy to review )
- Sort using statements (should be easy to review)
- Sort various element attempting to keep under a line limit (I would complete at least one file.) The link @iSazonov suggested says 400.
- Sometimes between coworkers, we use commits for breaking up reviews

markekraus · 2017-12-19T20:28:00Z

@TravisEz13 I actually haven't enabled stylecop. I tried but couldn't get it to work. I believe it would apply to the entire utility assembly which would make it very hard to track down just the web cmdlet code issues. This was all somewhat beyond me. So I resorted to sorting the code manually and using the stylecop rules as guidance. We can't use them all as, for example, our contributor guidelines say to group the private backing fields with public properties. that will make it hard to see what private field and private-before-public sorting issues are actually issues.

I can break it up, yes. I tried breaking up a large change into the multiple commits in a single PR before, but @iSazonov didn't like that. I don't care how the work is broken up, but I could use some guidance, clarity, and consistency. I can be told one thing in a PR/issue, follow that to the letter in a new PR, and then have reviewers telling me to redo it a different way. This pattern has repeated several times and results in a lot of wasted time for all involved.

So, should I break this out into separate PRs? Or should I do multiple commits in a single PR or should I group a few commits in a few PRs? Should I move the files first? start with the partial class merges?

TravisEz13 · 2017-12-19T21:39:53Z

@markekraus Can you point me to the previous PR with the objection from @iSazonov ?

markekraus · 2017-12-19T21:44:08Z

#5518 (comment)

TravisEz13 · 2017-12-19T22:03:38Z

I think we should try to take an incremental approach. One of the reasons to enable codefactor.io on your branch (maybe we should enable it after 6.0.0 is released) is to show that the PR was an overall improvement (did not introduce more issues than in removed,) not to say that you have to fix every issue.

I reviewed this link (https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/) further and I agree with the line limit and the reason behind it. I spend an hour reviewing this PR and completed a few hundred lines.

Is there a reason that we cannot do a simple merge of the partial classes without any sorting (perhaps you could sort the using statements?) This seems like a good starting point.

markekraus · 2017-12-19T22:16:19Z

Right now the pieces of the partial classes are in the same file. What constitutes a simple merge in this case? do I just move that code so it's in the same class brackets at the end of the class?

One reason why I was merging and sorting at the same time is that the classes are so out of alignment it's hard to even chose a logical place to move the partial code into. I can move the code directly in the end of the larger class, but then that code is all unsorted and that's sort of how it is now, only its in its own partial class block. If i try to move it with similar element types, I might have 2 to 3 places to chose from. If i try to move it to where it should be it will be out of place because the code around it is not where it should be.

TravisEz13 · 2017-12-19T22:40:41Z

Another approach would be to focus on a limited number of classes to keep under the line limit. I see one file that is over the line limit by itself. I'd say try this approach with the smaller files and we can proceed from there. @daxian-dbw @adityapatwardhan We should update the guidelines, but there should not be a requirement that a certain change is completed in a single PR unless splitting the change would have a negative effect on the project.

markekraus · 2017-12-19T22:50:32Z

Ok. I'm going to close this PR so I can keep the branch for reference. I turned on codefactor.io, but I'm not real happy with the full read/write access it requires. I will work on smaller chunks per PR.

[Feature] Sort and Merge Classes in WebCmdlets\Common

e58eaa1

markekraus requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners December 9, 2017 14:31

iSazonov approved these changes Dec 13, 2017

View reviewed changes

iSazonov assigned TravisEz13 Dec 16, 2017

markekraus closed this Dec 19, 2017

markekraus deleted the SortWebCmdletsCommon branch January 19, 2018 18:59

CarloToso mentioned this pull request Feb 9, 2023

WebCmdlets relocate to Common Path #19122

Closed

22 tasks

Sort and Merge Classes in WebCmdlets\Common #5662

Sort and Merge Classes in WebCmdlets\Common #5662

Uh oh!

Conversation

markekraus commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Dec 19, 2017

Uh oh!

markekraus commented Dec 19, 2017

Uh oh!

TravisEz13 commented Dec 19, 2017

Uh oh!

markekraus commented Dec 19, 2017

Uh oh!

TravisEz13 commented Dec 19, 2017

Uh oh!

markekraus commented Dec 19, 2017

Uh oh!

TravisEz13 commented Dec 19, 2017

Uh oh!

markekraus commented Dec 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markekraus commented Dec 9, 2017 •

edited

Loading