Sorting usings #923
Sorting usings #923
Conversation
belav
force-pushed
the
sort-usings
branch
3 times, most recently
from
9878a55
to
1084567
Compare
There was a problem hiding this comment.
I think there should be some test cases for code that does crazy things with comments, for example:
// Licensed to something
using Microsoft.AspNetCore;
// I like putting
using Microsoft;
/*
* random comments everywhere
*/
using Apple;
/* because it's valid C# code */
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
// Look mom, I'm using a really fancy authentication library:
using Microsoft.Identity.Web.UI;
using Microsoft.Identity.Web; // another comment just for good measureI'm unsure how this case should be handled, these are the options I could think of:
- Keep everything as is, whoever put a comment between the using statements probably has a good reason (Maybe show a warning when formatting? I am not familiar with this codebase so i'm not sure if showing warnings is even possible)
- Sort the individual blocks of usings between the comments
- Be opinionated and just remove the comments, lol
As far as I can tell tell it's impossible to sort usings with comments while preserving the original intent of the comments in all cases.
I don't really care what the final solution is, just that it's tested and works with all the possible comment formats.
There was a problem hiding this comment.
Keeping comments at the top was the only tricky part. Most comments are leading trivia which just goes with the node it is above. Trailing comments also stick with the node they are after. From all the code I reviewed, comments in usings aren't that common. #if is hard to deal with, but I have the basic cases covered. The .net analyzers themselves have some open issues around #ifs, so I'm not worried about the couple of edge cases that I couldn't resolve which appeared in the mono code.
// Licensed to something
/*
* random comments everywhere
*/
using Apple;
// I like putting
using Microsoft;
using Microsoft.AspNetCore;
/* because it's valid C# code */
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
using Microsoft.Identity.Web; // another comment just for good measure
// Look mom, I'm using a really fancy authentication library:
using Microsoft.Identity.Web.UI;
|
|
||
| using First; | ||
| using Second; | ||
| B; |
There was a problem hiding this comment.
I believe that was me going nuts trying comments everywhere a while back.
Apparently that is valid c#!
| } | ||
| } | ||
|
|
||
| yield return globalUsings.OrderBy(o => o.Using!, Comparer).ToList(); |
There was a problem hiding this comment.
Why do you need the null suppression operator for o.Using!?
.OrderBy() usually doesn't need it.
There was a problem hiding this comment.
Looks like because I was missing the ? on IComparer<UsingDirectiveSyntax?>
| yield return globalUsings.OrderBy(o => o.Using!, Comparer).ToList(); | ||
| yield return systemUsings.OrderBy(o => o.Using!, Comparer).ToList(); | ||
| yield return aliasNameUsings.OrderBy(o => o.Using!, Comparer).ToList(); | ||
| yield return regularUsings.OrderBy(o => o.Using!, Comparer).ToList(); |
There was a problem hiding this comment.
I suspect if you just do .OrderBy(o => "" + o.Using?.Alias.ToFullString() + o.Using?.Name.ToFullString()), you won't need that comparer.
Though it's long enough that it might be worth extracting to a helper method, and it might be slower because of the string concat, so 🤷
closes #661