Sort namespaces as per stylecop rules/.net convention and remove unused namespaces from source files. #1963

codemzs · 2018-12-24T01:38:33Z

Sorts namespaces as per stylecop rules/.net convention and removes unused namespaces from source files. Standardizes namespace to be always sorted as per .NET convention in source files.

My impressions after doing this change:

There is decent amount of code under IF DEFs that seems dead and should be removed, example:
In IntArray.cs
#if USE_SINGLE_PRECISION
using FloatType = System.Single;
#else
using FloatType = System.Double;
#endif

USE_SINGLE_PRECISION is not defined anywhere and FloatType is not at all used anywhere in the file. We should consider opening an issue to do this sort of clean up. There are many such examples.

Consider removing Float and use float directly.

CC: @TomFinley

fixes #1961
fixes #1962

…amespaces from source files.

justinormont · 2018-12-24T02:12:50Z

src/Microsoft.ML.Core/Utilities/MathUtils.cs

 using System;
 using System.Collections.Generic;
-using System.Linq;
+using Float = System.Single;


Should we rename Float in the file to Single? It seems odd having our own alias of float/Single.

Seems we do this in many of the files... (not just this one file)

May be we should but does it make sense to be it owns change so that its easier on the reviewers? Apparently there are 54 files where Float = System.Single is being used so I suspect removing them all and changing all their references to float might pollute this change. Lets see what @TomFinley feels? I'm fine either ways.

It's unclear to me why we're bringing that up in this PR. There is no urgency to that change. We've been doing it opportunistically (not that exactly, but Float to float). Let's continue that policy. Doing it all at once would cause a great deal of conflicts with existing changes for no particular reason whatsoever. (These changes being at the header of the file are at least easier to manage.)

Agreed.

I initially didn't notice using Float = System.Single; was used in many files. If it was only in one, I'd ask for the change. It's a larger tasks which belongs in its own PR.

sharwell

If you want to standardize on the format used in this pull request, I would expect to see the following changes as well:

A .editorconfig is added which sets the placement of System using directives for C# source files:

# Sort using and Import directives with System.* appearing first
dotnet_sort_system_directives_first = true

A stylecop.json file is added, which sets the expected using directives placement to outside the namespace

{
  "$schema": "https://raw.githubusercontent.com/DotNetAnalyzers/StyleCopAnalyzers/master/StyleCop.Analyzers/StyleCop.Analyzers/Settings/stylecop.schema.json",
  "settings": {
    "orderingRules": {
      "usingDirectivesPlacement": "outsideNamespace"
    }
  }
}

The analyzers for enforcing using directives ordering (SA1208, SA1209, SA1210, SA1211, SA1216, SA1217) and placement (SA1200) are enabled

machinelearning/src/Source.ruleset

Lines 148 to 151 in 0c62e30

    
           <Rule Id="SA1208" Action="None" /> 
        
           <Rule Id="SA1209" Action="None" /> 
        
           <Rule Id="SA1210" Action="None" /> 
        
           <Rule Id="SA1211" Action="None" />

machinelearning/src/Source.ruleset

Lines 155 to 156 in 0c62e30

    
           <Rule Id="SA1216" Action="None" /> 
        
           <Rule Id="SA1217" Action="None" />

codemzs · 2018-12-24T02:44:57Z

@sharwell This is great feedback and I was thinking the same how we could standardize, thanks. Do you by any chance have an example of a repo that contains such standardization? i.e use of .editorconfig and stylecop.json

codemzs · 2018-12-24T04:56:08Z

@sharwell Thanks for reaching out on IM today evening and helping me with standardizing stylecop. I found some errors using it that I had earlier missed, not sure how even though I used VS's power tools extensions to sort namespaces and remove unused ones.

sharwell

💡 I forgot to include SA1200 in the set of diagnostics that can be enabled in the ruleset file, but that's less important than the other 6 you already included and could easily be part of a future change.

sharwell · 2018-12-24T05:52:54Z

src/Microsoft.ML.FastTree/SumupPerformanceCommand.cs


 [assembly: LoadableClass(typeof(SumupPerformanceCommand), typeof(SumupPerformanceCommand.Arguments), typeof(SignatureCommand),
    "", "FastTreeSumupPerformance", "ftsumup")]

 namespace Microsoft.ML.Trainers.FastTree
 {
-    using Stopwatch = System.Diagnostics.Stopwatch;
+using Stopwatch = System.Diagnostics.Stopwatch;


😕 not sure how this indentation change happened.

TomFinley · 2018-12-25T04:06:42Z

Hi @codemzs, may I ask how we automated this change? My reasons for wondering are, I feel like in a PR like this it would be more helpful to review how we automated, rather than review every single file.

At least, assuming you did automate. 😄 Maybe you didn't.

codemzs · 2018-12-25T04:15:03Z

Hi @TomFinley , I used VS’s powertools extension and ran it on every project, compiled it, fixed extra lines errors and repeated the process. Lastly I enforced namespaces be sorted via editor configuration using the method @sharwell suggested, this also verified the namespaces are sorted as per .NET standards outlined in this PR’s description.

TomFinley · 2018-12-25T04:27:50Z

Hi @TomFinley , I used VS’s powertools extension and ran it on every solution, compiled it, fixed extra lines errors and repeated the process. Lastly I enforced namespaces be sorted via editor configuration using the method @sharwell suggested, this also verified the namespaces are sorted as per .NET standards outlined in this PR’s description.

Excellent. Thank you @codemzs (and @sharwell !).

TomFinley

Thanks @codemzs

codemzs · 2018-12-25T04:29:37Z

Thank you for reviewing and approving, @TomFinley and @sharwell !

Anipik

LGTM

codemzs · 2018-12-25T06:47:41Z

Thanks @Anipik for reviewing!

Sort namespaces as stylecop rules/.net convention and remove unused n…

67d51db

…amespaces from source files.

codemzs requested review from sfilipi, TomFinley and justinormont December 24, 2018 01:38

codemzs changed the title ~~Sort namespaces as stylecop rules/.net convention and remove unused namespaces from source files.~~ Sort namespaces as per stylecop rules/.net convention and remove unused namespaces from source files. Dec 24, 2018

restore namespaces for CPUMath perf tests(needed by core builds).

1175a5e

justinormont reviewed Dec 24, 2018

View reviewed changes

sharwell requested changes Dec 24, 2018

View reviewed changes

codemzs added 2 commits December 23, 2018 19:54

standardize stylecop namespace sorting rules.

b169118

fix core build.

3189a23

sharwell approved these changes Dec 24, 2018

View reviewed changes

codemzs requested review from abgoswam, wschin and ganik December 24, 2018 17:10

TomFinley approved these changes Dec 25, 2018

View reviewed changes

codemzs requested review from artidoro and Anipik December 25, 2018 06:27

Anipik approved these changes Dec 25, 2018

View reviewed changes

codemzs merged commit a570da1 into dotnet:master Dec 25, 2018

ghost locked as resolved and limited conversation to collaborators Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort namespaces as per stylecop rules/.net convention and remove unused namespaces from source files. #1963

Sort namespaces as per stylecop rules/.net convention and remove unused namespaces from source files. #1963

codemzs commented Dec 24, 2018 •

edited

Loading

justinormont Dec 24, 2018

justinormont Dec 24, 2018 •

edited

Loading

codemzs Dec 24, 2018

TomFinley Dec 25, 2018 •

edited

Loading

justinormont Dec 25, 2018

sharwell left a comment •

edited

Loading

codemzs commented Dec 24, 2018 •

edited

Loading

codemzs commented Dec 24, 2018

sharwell left a comment

sharwell Dec 24, 2018

TomFinley commented Dec 25, 2018 •

edited

Loading

codemzs commented Dec 25, 2018 •

edited

Loading

TomFinley commented Dec 25, 2018

TomFinley left a comment

codemzs commented Dec 25, 2018

Anipik left a comment

codemzs commented Dec 25, 2018

	<Rule Id="SA1208" Action="None" />
	<Rule Id="SA1209" Action="None" />
	<Rule Id="SA1210" Action="None" />
	<Rule Id="SA1211" Action="None" />

	<Rule Id="SA1216" Action="None" />
	<Rule Id="SA1217" Action="None" />

Sort namespaces as per stylecop rules/.net convention and remove unused namespaces from source files. #1963

Sort namespaces as per stylecop rules/.net convention and remove unused namespaces from source files. #1963

Conversation

codemzs commented Dec 24, 2018 • edited Loading

justinormont Dec 24, 2018

Choose a reason for hiding this comment

justinormont Dec 24, 2018 • edited Loading

Choose a reason for hiding this comment

codemzs Dec 24, 2018

Choose a reason for hiding this comment

TomFinley Dec 25, 2018 • edited Loading

Choose a reason for hiding this comment

justinormont Dec 25, 2018

Choose a reason for hiding this comment

sharwell left a comment • edited Loading

Choose a reason for hiding this comment

codemzs commented Dec 24, 2018 • edited Loading

codemzs commented Dec 24, 2018

sharwell left a comment

Choose a reason for hiding this comment

sharwell Dec 24, 2018

Choose a reason for hiding this comment

TomFinley commented Dec 25, 2018 • edited Loading

codemzs commented Dec 25, 2018 • edited Loading

TomFinley commented Dec 25, 2018

TomFinley left a comment

Choose a reason for hiding this comment

codemzs commented Dec 25, 2018

Anipik left a comment

Choose a reason for hiding this comment

codemzs commented Dec 25, 2018

codemzs commented Dec 24, 2018 •

edited

Loading

justinormont Dec 24, 2018 •

edited

Loading

TomFinley Dec 25, 2018 •

edited

Loading

sharwell left a comment •

edited

Loading

codemzs commented Dec 24, 2018 •

edited

Loading

TomFinley commented Dec 25, 2018 •

edited

Loading

codemzs commented Dec 25, 2018 •

edited

Loading