Open vsfeedback opened 4 years ago
weitzhandler
commented
4 years ago Here are some things to pay attention.
Microsoft / Windows / Xamarin namespaces as System ones?I'd be happy to contribute given some guidance.
Enderlook
commented
4 years ago Hi @weitzhandler, it's my first time both contributing with code at some open-source repository and a codebase of this size, so if I'm doing something wrong please tell me.
I found this has help wanted label, so I have been trying to implement it. However, I got stuck and have a few questions.
This is the file I edited to implement my changes, I commented it as best I could: https://github.com/Enderlook/roslyn/blob/improve-sort-using-directives/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/TokenComparer.cs
The current modifications allow to order by priority:
However:
specialCaseSystem and SystemFirstInstance in TokenComparer (and all places where this names are used, like UsingsAndExternAliasesDirectiveComparer.SystemFirstInstance) are no longer correct. I mean, that names should now be called something like "specialCaseNamespaces" because it's no longer only System.*, but also Microsoft.*, Windows.*, Xamarin.* and maybe user-defined namespaces. I tried to change its name, but that means editing hundred of files and I am not sure about their side effects. Also, when reaching file src\Workspaces\SharedUtilitiesAndExtensions\Compiler\Core\Editing\GenerationOptions.cs, I lose track of how to edit that boolean option name, so I didn't change the names.TokenComparer allows only to compare tokens, and a token doesn't have enough information to determine if the namespace comes from the current project, solution or NuGet package. So no clue of how to implement that feature.TokenComparer file and class in VBA, though I am not exactly familiar with the language. Although it's too never late to learn a new language... if that is the case I think it would be easier to first implement the feature in C#, and then port it to Visual Basic once it's done, right?By the way. I would be developing this in my free time, after studying for exams, so if there is any time constrain to implement this feature I'm not sure if would be able to do it. Also, never worked with so advanced code, I hope have not chosen an issue that outtask the scope of my skills :blush:...
CyrusNajmabadi
commented
4 years ago hi @Enderlook . You're definitely welcome to take a PR here. However, i can't guarantee we would take it. Specifically, i think we would want to hammer out as a team what we would the design to be here. Important questions we would want are:
That said, if you want to still go on and create a PR, def feel free to. Just keep expectations in line.
Because of your interest in this issue, i am willing to bump this to our next design meeting (monday) to get some answers on this topic. You're welcome to wait until that happens, or just go ahead if you're interested.
CyrusNajmabadi
commented
4 years ago In terms of your questions:
CyrusNajmabadi
commented
4 years ago I've moved this to the next meeting for us to discuss things based on your interest.
CyrusNajmabadi
commented
4 years ago Hi @Enderlook. We discussed this today and tentatively approved having work done in this area, given an appropriate specification signed off by the team. I'm going to write that speclet (immediately) and then ensure the team is ok with it (which hopefully won't take too long). Once we hammer things out to a point we're satisfied, we'll let you know.
CyrusNajmabadi
commented
4 years ago A new editor-config option will be provided (name TBD, but likely something akin to dotnet_import_directives_custom_order) to allow users to flexibly decide on both import sorting and import spacing (i.e. the number of newlines between using groups).
The expected format would be similar to:
dotnet_import_directives_order="System.*\nMicrosoft.*\nWindows.*\n\n*\n\nXamarin.*"This would be equivalent to the following:
System.*
Microsoft.*
Windows.*
*
Xamarin.*The items in the string need to be:
.*. i.e. System.*, System.IO.*. These designate an import group.* . This also designates an import group. It's effectively the degenerate case of '1'.\n*'s are not allowed like so System.*.Generic.
Within any group, imports are sorted alphabetically the same way as we do today.
To determine which group an import belongs to, look through all the groups for the 'best' one. 'Best' is defined as matching the most number of name-segments between the import and the pattern. So if we had:
*
System.*
System.IO.*Then System.IO.Stream would match the last item because it matched two name segments. This would also be true if there was:
*
System.IO.*
System.*i.e. it's not matching the last/first item, but the best item.
* on its own effectively means that zero segments matched anything else, and thus is where items will go if nothing else matched.
Without
The above patterns specify the different groups that imports will be placed into. Each group will be separated from other groups based on the number of \n entries between them. A single \n means the groups should touch. i.e.: System.*\nMicrosoft.* means we will end up with
System.Collections.X;
System.Y.Z;
Microsoft.Foobar;
Microsoft.Goobar;Whereas System.*\n\nMicrosoft.* would be
System.Collections.X;
System.Y.Z;
Microsoft.Foobar;
Microsoft.Goobar;We already have dotnet_sort_system_directives_first and dotnet_separate_import_directive_groups options that have shipped. If we are sorting/spacing the imports for some file, and it has the new dotnet_import_directives_custom_order option, then that new option supersedes the other two options. There is no interplay between the new option and the old options.
Indeed, you can think of the old options meaning the following in terms of the new option:
| dotnet_sort_system_directives_first | dotnet_separate_import_directive_groups | dotnet_import_directives_custom_order |
|---|---|---|
false |
false |
"*" |
false |
true |
not supported** |
true |
false |
"System.*\n* |
true |
true |
"System.*\n\n*** |
** @sharwell this raises a bit of an interesting issue with the match all pattern. It can form a group to grab everything else, but there's no way to state that within that group we should have general spacing. Is that an issue?
Do we need special UI in VS for this? Probably. Not only is the editorconfig option hard to find, it would also be somewhat hard to write given the way \ns work in it. You really want the user to have a nice list-box to specify things. However, it could be a lot of work given the complexity of the UI. So the team should decide how necessary that is. It can also come at a later point in time.
We'll have to go through and remove/update a lot of places taht currently pass the two existing booleans everywhere. We'll likely have to create a rich internal object that encapsulates all this data and pass that instead.
There are open questions that we can save to the implementation phase.
using global::System.IO should that match System.IO or not? I believe based on the current impl it does, we should probably preserve that. global:: is to ensure the right name is looked up, but people still think of this as the System.IO namespace not the gxxxx namespace.Enderlook
commented
4 years ago Thank for the specifications @CyrusNajmabadi. I got some ordering questions:
System.* matches also System (even when it doesn't have .)?* without a .?
For example: System.Co* would match all of (in this order):
\n at the beginning or end?
For example:
dotnet_import_directives_order="\nSystem.*\nMicrosoft.*\nWindows.*\n\n*\n\nXamarin.*\n"
Would they be allowed?*, would all unmatched namespaces go to end?CyrusNajmabadi
commented
4 years ago Does, for example, System.* matches also System (even when it doesn't have .)?
Yes
Is valid to place * without a .?
No.
What does means \n at the beginning or end?
Not certain. I don't see much harm in respecting it. But i also don't see much value in it.
If the user doesn't write *, would all unmatched namespaces go to end?
Yes. That seems reasonable.
--
Great questions!
Enderlook
commented
4 years ago Sorry, more questions 😄
Shall it silently fix:
System..IO as System.IO.System.IO. as System.IO.*.System.IO* as System.IO.*.System.IO as System.IO.*.System.Foo*.Bar as System.Foo.Bar. Also:
"System.*\nMicrosoft.*\nWindows.*\nXamarin.*\n*\System.*"
If any error should be raised, how should I handle them, by throwing an exception or is there any logging for that?
If the user doesn't write * all unmatched namespaces go to end as decided in your previous comment.
\ns at the end of the configuration?
I.e:
Append (preserve) \n at the end if there is no lonely
"System.*\nMicrosoft.*\nWindows.*\nXamarin.*" -> "System.*\nMicrosoft.*\nWindows.*\nXamarin.*\n*"
"System.*\nMicrosoft.*\nWindows.*\nXamarin.*\n" -> "System.*\nMicrosoft.*\nWindows.*\nXamarin.*\n\n*"Or should it ignore (replace) them?
// Replace all leading \n with a single \n*
"System.*\nMicrosoft.*\nWindows.*\nXamarin.*\n\n\n" -> "System.*\nMicrosoft.*\nWindows.*\nXamarin.*\n*"
"System.*\nMicrosoft.*\nWindows.*\nXamarin.*" -> "System.*\nMicrosoft.*\nWindows.*\nXamarin.*\n*"As you pointed out later:
** @sharwell this raises a bit of an interesting issue with the
match allpattern. It can form a group to grab everything else, but there's no way to state that within that group we should have general spacing. Is that an issue?
- However, adding a new keyword
#would make that possible! For example:System.*\n#\nMicrosoft.*Order as:
System.Collection; System.IO;
MyLibrary.A MyLibrary.B
YourLibrary.X YourLibrary.Y
ThatLibrary
Microsoft.CodeAnalysis
Although, this raises two additional questions:
1. What happens if you write both `#` and `*`? I.e: `System.*\n#\nMicrosoft.*\n*`
Maybe they could be treated as duplicated entries and follow the same rules in question 6.
2. If the user doesn't write `*` nor `#`, should we add a `*` or `#` at the end of the configuration? I.e: unmatched namespaces always go to end... but are they split into groups?
## Implementation
### TokenComparer
The type `TokenComparer` compares namespace token by token (at least that I understand reading the source code).
I mean, for example, in order to compareSystem.Collection.Generic System.Collection
The `TokenComparer` compares part by part:
1. `System` vs `System`.
2. `Collection` vs `Collection`.
3. `Generic` vs nothing.
That did work because all `System.*` namespaces were together.
However, the new configuration allows to sort namespaces with common names at different orders, like:System. Microsoft. System.IO. Windows.
So, using the old scheme, this would produce false results.
I found (if I'm not wrong) that namespaces are decomposed in tokens during `NameSyntaxComparer.Compare` [here](https://github.com/dotnet/roslyn/blob/a38625c172c73d5257f387bc949873cd7d67ce2a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/NameSyntaxComparer.cs#L93:L103)
<details>
<summary>Snippet</summary>
```cs
var xNameParts = DecomposeNameParts(x);
var yNameParts = DecomposeNameParts(y);
for (var i = 0; i < xNameParts.Count && i < yNameParts.Count; i++)
{
var compare = Compare(xNameParts[i], yNameParts[i]);
if (compare != 0)
{
return compare;
}
}However, at the moment I'm not fully understanding exactly why the method is recursive.
If there were a way to get the whole directive as a string, array or similar, like "System.Collection.Generic" or ["System", "Collection", "Generic"] that would simplify the ordering, instead of checking this token by token. But if I change that code I am not sure if this may break other things that I'm not aware since I don't know if NameSyntaxComparer is only in charge of sorting using directives or also does more things (for example, this type has support to sort GenericNameSyntax but as far I know a namespace can't be generic, so I guess this compares more than only namespaces).
Maybe I could rewrite the logic of both TokenComparer and NameSyntaxComparer Compare methods, but for a change of that magnitude and I'm not sure if I must ask for allowance/guidance before.
I was thinking the comparison logic could work something like this non-tested pseudocode (still should see how to adapt with existing code, like with GenericNameSyntax):
I haven't done any research yet, I guess implementing that after implementing the ordering will be easier.
[...] And there will very much be open questions about how to pass this information around [...]
I already made a small type to process that configuration string into something more "usable" (the sorted array from above). However, I don't know how this object will connect with the existing code.
We'll have to go through and remove/update a lot of places that currently pass the two existing booleans everywhere. We'll likely have to create a rich internal object that encapsulates all this data and pass that instead.
From where that object will come from? I managed to track current configurations options up to GenerationOptions, however, I don't know how configuration is retrieved from there, reading some code from random files I believe it may be Document.GetOptionsAsync() but not sure where can I get a Document type.
I'm sorry for the wall text 😊
CyrusNajmabadi
commented
4 years ago Shall it silently fix:
... multiple dots? Like, interpret System..IO as System.IO.
No. Let's be strict. There's little value to being loose. And it means we have a compat bar to maintain in the future if we want to change things which could be anywhere from annoying to downright painful.
... missing asterisk? Like, interpret System.IO. as System.IO.*.
No.
... missing dot? Like, interpret System.IO as System.IO..
No.
... missing asterisk and dot? Like, interpret System.IO as System.IO.*.
No.
... invalid asterisks? Like, interpret System.Foo*.Bar as System.Foo.Bar.
Definitely not.
--
In the future we could decide to relax these if we feel there is very high value. But i don't personally think there is very high value. You write this once (or use a UI to spit it out), and that should be it. Having a strict and simple format makes it easy to documetn, explain, support and maintain.
CyrusNajmabadi
commented
4 years ago
- How are handled duplicated entries? I.e: "System.\nMicrosoft.\nWindows.\nXamarin.\n\System."
I've fine with it being an error, or just having any behavior.
CyrusNajmabadi
commented
4 years ago Missing If the user doesn't write all unmatched namespaces go to end as decided in your previous comment.
- However, should this take into account \ns at the end of the configuration?
I would say no. if you have no trailing \n, then:
System.*
Microsoft.*Is effectively
System.*
Microsoft.*
*If you want multiple blank lines at the end you need to write:
System.*
Microsoft.*
*i.e. you need to manually provide the \n\n*.
CyrusNajmabadi
commented
4 years ago However, adding a new keyword # would make that possible! For example:
Hold off on that for now. We'll bring to next design meeting to decide on what to do. I'll bring up your suggestion :)
CyrusNajmabadi
commented
4 years ago Maybe I could rewrite the logic of both TokenComparer and NameSyntaxComparer Compare methods, but for a change of that magnitude and I'm not sure if I must ask for allowance/guidance before.
You can rewrite them. THey're not heavily used components. They basically exist almost entirely for this feature.
CyrusNajmabadi
commented
4 years ago I was thinking the comparison logic could work something like this non-tested pseudocode.
My recommendation is you do this in a TDD style. So write a ton of test cases that validate all the different behaviors, then go in and try to write the code. I imagine you'll find it's either easy or hard. And, if hard, you'll likely finally get it working, then see a lot of opportunity to cleanup/simplify once yuo figure out how to handle all cases.
New lines. I haven't done any research yet, I guess implementing that after implementing the ordering will be easier.
Yes. i woudl recommend sorting first, then doing grouping. Note that we already do that today, it's just that things are more 'hardcoded'. So the general high level algorithm should still apply, just that the specific logic for determining groups will differ now.
CyrusNajmabadi
commented
4 years ago From where that object will come from? I managed to track current configurations options up to GenerationOptions, however, I don't know how configuration is retrieved from there, reading some code from random files I believe it may be Document.GetOptionsAsync() but not sure where can I get a Document type.
Basically, it's pretty simply to define EditorConfigOptions. Here are the important bits:
public static readonly PerLanguageOption2<bool> PlaceSystemNamespaceFirst = new PerLanguageOption2<bool>(nameof(GenerationOptions),
CodeStyleOptionGroups.Usings,
nameof(PlaceSystemNamespaceFirst), defaultValue: true,
storageLocations: new OptionStorageLocation2[] {
EditorConfigStorageLocation.ForBoolOption("dotnet_sort_system_directives_first"),
new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.PlaceSystemNamespaceFirst")});PerLanguageOption2<bool>The type of hte option. Here you'd do the same, except instead of bool you'd have your rich immutable type that encoded this information.
PlaceSystemNamespaceFirst The name of the option internally, call it something reasonable
nameof(GenerationOptions),
CodeStyleOptionGroups.Usings,Just keep these the same.
defaultValue: true,The default value for this. For this, this should likely be null.
EditorConfigStorageLocation.ForBoolOption("dotnet_sort_system_directives_first"),Defines the name of the editroconfig key, as well as the code that loads the value for it. In this case though you'll have to write your own special parser as this is a totally new string encoding for a value. You'll replace this with:
new EditorConfigStorageLocation<YourNewType>("dotnet_whatever_we_decided_on", YourParseMethod, YourSerializationMethod);Hopefully that's enough to go off of :)
CyrusNajmabadi
commented
4 years ago Ok, i think i've answered everything :)
Enderlook
commented
4 years ago Thanks for that responses! You missed:
[When parsing the input data] If any error should be raised, how should I handle them, by throwing an exception or is there any logging for that?
Also:
My recommendation is you do this in a TDD style
In which project should I put my tests? Or they are not stored in Roslyn solution?
Thanks!
CyrusNajmabadi
commented
4 years ago [When parsing the input data] If any error should be raised, how should I handle them, by throwing an exception or is there any logging for that?
Don't throw. Just return null for now indicating that no valid value was specified. Yes, this sucks for the user. We have an open work item about tracking and reporting errors in editorconfig to users.
CyrusNajmabadi
commented
4 years ago In which project should I put my tests? Or they are not stored in Roslyn solution?
OrganizeUsingsTests and OrganizeImportsTests
Note: this will need to be done for VB as well. But that can come once the design/impl is finalized for C# :)
Enderlook
commented
4 years ago [When parsing the input data] If any error should be raised, how should I handle them, by throwing an exception or is there any logging for that?
Don't throw. Just return
nullfor now indicating that no valid value was specified. Yes, this sucks for the user. We have an open work item about tracking and reporting errors in editorconfig to users.
So if a part of the configuration is wrong we just use the default configuration? Got it!
CyrusNajmabadi
commented
4 years ago I have a slightly different proposal i would like to bring to the design meeting for next monday. It is specifically designed to address some of the gaps i found in the original proposal which were placed into the table here:
Namely that it's hard for this new option to fully replace the old one. That's slightly unpalatable as i would not want to replace the existing system with one that couldn't fully handle all the cases it does in the same manner as today..
The question is how to encode that you want System first, with newlines between top level groups?. There are two solutions i can think of (but design meeting may result in more).
Have a different sigil/encoding to say "catch all, with newlines between top-level caught group". For example, instead of System.*\n\n* you would say System.*\n\n#. The former would mean "there is a catch-all group, without newlines in that group", the latter would say "tehre is a catch all group, and it should itself be split up by blank lines."
Pros: Seems like it would handle this case. Cons: The format (which is already ugly) just seems to be getting uglier and more confusing. The first specification at least is easy to understand, this new one is just getting strange.
Take newline-grouping out of the new format. Instead, you would just say something like System.* ; * or System.* ; Microsoft.* ; Xamarin.* ; *. i.e. a semicolon delimited list of patterns. This would be used solely for the ordering. We would then keep around and respect the existing dotnet_separate_import_directive_groups:true/false option to determine if there should be a blank line between the groups.
Note: we could also pick another delimiter, or just allow whitespace. For example:
System.* ; *
b. System.* ; Microsoft.Xyz.* ; Microsoft.* ; Xamarin.*
c. System.* ; Microsoft.Xyz.* ; Microsoft.* ; * ; Xamarin.*System.* , *
b. System.* , Microsoft.Xyz.* , Microsoft.* , Xamarin.*
c. System.* , Microsoft.Xyz.* , Microsoft.* , * , Xamarin.*System.* *
b. System.* Microsoft.Xyz.* Microsoft.* Xamarin.*
c. System.* Microsoft.Xyz.* Microsoft.* * Xamarin.*Pros:
\n in the string is unpleasant on many levels.Cons:
System.*\nMicrosoft.*\n\n*. I.e. group everything System/MS together, and everything else together, and keep those two groups separated by a blank line.It's unclear to me if the con is a problem though. We've never supported that before anyways, and i think the ordering is more important to users than the fine-grained spacing.
CyrusNajmabadi
commented
4 years ago Anotehr proposal i have is just to drop the .* for these patterns (except for the catch-all). Because they show up at the end, and are required, they seem redundant. This would mean you could then say:
System ; *
b. System ; Microsoft.Xyz ; Microsoft ; Xamarin
c. System ; Microsoft.Xyz ; Microsoft ; * ; XamarinSystem , *
b. System , Microsoft.Xyz , Microsoft , Xamarin
c. System , Microsoft.Xyz , Microsoft, * , XamarinSystem *
b. System Microsoft.Xyz Microsoft Xamarin
c. System Microsoft.Xyz Microsoft * XamarinGithubs lists trim whitespace. But i think you would write this as the following:
System Microsoft.Xyz Microsoft * XamarinWhich i think is very readable for anyone.
Enderlook
commented
4 years ago If you want flexibility, yet a readable configuration, something like this may be done:
System.Collection, Microsoft 3, System, * 1 * 2, Xaramin
Which means:
System.Collections
System.Collection.Generics
Microsoft
Microsoft.CodeAnalysis
System
System.IO
Foo.A
Foo.B
Bar.A
Bar.B
XamarinSystem.Collection), no space is added.Microsoft 3), that is the number of newlines between each group.* is a special case:
* or * 4, is interpreted as above.* followed by a number, another * and optional number, such as * 2 * or * 1 * 4 means: Between each group add X amount of lines, [if the other number is present] after all these groups, add Y amount of lines.After all, as far I know, namespaces can't be numbers so these numbers should no be a problem.
Though, the special treatment of * may look weird.
Just a humble idea
CyrusNajmabadi
commented
4 years ago Just a humble idea
The idea is definitely good :)
--
Here's where i stand:
I have no doubt we could come up with an encoding that represents the fully flexible model of what people could want to say.
However, i'm skeptical we can do so in a way that provides a simple, clear and pleasant user-facing config string. i.e. the above string you specified could work, but it's just 'goop' IMO. i.e. it's the encoding result of some UI, not something i would like to tell users to edit themselves.
My preference is to have a really clean appraoch that is just braindead simple and readable, which hits the 98% case. I'm positing that System Microsoft.Xyz Microsoft * Xamarin along with dotnet_separate_import_directive_groups:true/false is sufficient/appropriate for that.
CyrusNajmabadi
commented
4 years ago I will bring your suggestion to the meeting as well. We'll hash it out and decide where to land. I can say right now that i'm not a fan of the first initial proposal we came up with. It's a little too noisy and redundant. And it doesn't seem like a complete solution. I'm hoping we can come up with something that balances out all these concerns :)
CyrusNajmabadi
commented
4 years ago Superceded. See https://github.com/dotnet/roslyn/issues/44052#issuecomment-647828287
CyrusNajmabadi
commented
4 years ago @Enderlook We further discussed and refined the proposal. The new rules are:
Design decision:
We will take option 1 as the form preferred. i.e.:
System;Microsoft.Xyz;Microsoft;*;XamarinNote that whitespace is allowed, so the above can be written as:
System ; Microsoft.Xyz ; Microsoft ; * ; XamarinIt is also possible to provide either * or ** (explained below). i.e.
System ; Microsoft.Xyz ; Microsoft ; * ; Xamarin
System ; Microsoft.Xyz ; Microsoft ; ** ; Xamarin* and ** means:
* that is a single group that should stay together.** that match will be broken into groups based on the first part of the import. i.e. if ** matched using Foo.X; using Foo.Y; using Moo.X; using Moo.Y, then that would be two groups Foo and Moo.dotnet_separate_import_directive_groups=true will break up groups as before. i.e. one blank line between groups.* or ** group, a ** group is implicitly added at the end. So System ; ** and System are equivalent.This makes the existing dotnet_sort_system_directives_first=true equivalent to System; ** or System and the existing dotnet_sort_system_directives_first=false equivalent to ** or `` (empty).
CyrusNajmabadi
commented
4 years ago By example, imagine you have
using System.Collections;
using System.Data;
using Microsoft.Windows;
using Newtownsoft.Json;
using Xamarin;
using Xamarin.Forms;Then
System ; Microsoft; * and dotnet_separate_import_directive_groups=true would produce:
using System.Collections;
using System.Data;
using Microsoft.Windows;
using Newtownsoft.Json;
using Xamarin;
using Xamarin.Forms;Whereas
System ; Microsoft; ** and dotnet_separate_import_directive_groups=true would produce:
using System.Collections;
using System.Data;
using Microsoft.Windows;
using Newtownsoft.Json;
using Xamarin;
using Xamarin.Forms;
logiclrd
commented
4 years ago Is there a way, with the current plan for this, to make it automatically group things at the second level?
I created this issue: https://github.com/dotnet/roslyn/issues/47802
This was just closed as a duplicate of this one, which it may very well be, but I just want to make sure that my use case is in fact fully captured here. Specifically, if I'm importing a whole bunch of namespaces that all start with a common first component, is there a way to automatically split apart different values for the second component into separate groups?
To put it another way, can I have it format this:
using Foo.Common;
using Foo.GronkulatorService.Contracts;
using Foo.GronkulatorService.Model;
using Foo.TextProcessing;
using Foo.TextProcessing.Markdown;
using Foo.TextProcessing.RegularExpressions;...as this:
using Foo.Common;
using Foo.GronkulatorService.Contracts;
using Foo.GronkulatorService.Model;
using Foo.TextProcessing;
using Foo.TextProcessing.Markdown;
using Foo.TextProcessing.RegularExpressions;And, can I do it without having to explicitly enumerate Common, GronkulatorService and TextProcessing in the configuration?
CyrusNajmabadi
commented
4 years ago And, can I do it without having to explicitly enumerate Common, GronkulatorService and TextProcessing in the configuration?
the currently design is that you would have to make groups for all of those. should be an initial setup on your part, and then a rare namespace added there.
logiclrd
commented
4 years ago I don't want to have to repeat that in every project that was previously configured every time I see a new second-level namespace. I'd like to request a small amendment to the specification, so that the ** specification can be qualified.
Ideally, I'd like to be able to say that Foo.X and Foo.Y should be different groups without forcing Foo's position in the ordering. So, I propose that the core syntax be **(list_of_roots), where list_of_roots is a comma-delimited list of namespaces from which children should be separate groups, with * meaning that anything unmatched should be included as well. With this, ** would be shorthand for **(*).
Input for examples:
using Microsoft.Windows;
using Newtonsoft.Json;
using System.Collections;
using System.Data;
using Test.Common;
using Test.GronkulatorService.Contracts;
using Test.GronkulatorService.Model;
using Test.TextProcessing;
using Test.TextProcessing.Markdown;
using Test.TextProcessing.RegularExpressions;
using Xamarin;
using Xamarin.Forms;Example: System;*
using System.Collections; using System.Data; using Microsoft.Windows; using Newtonsoft.Json; using Test.Common; using Test.GronkulatorService.Contracts; using Test.GronkulatorService.Model; using Test.TextProcessing; using Test.TextProcessing.Markdown; using Test.TextProcessing.RegularExpressions; using Xamarin; using Xamarin.Forms;
Example: System;**
using System.Collections; using System.Data; using Microsoft.Windows; using Newtonsoft.Json; using Test.Common; using Test.GronkulatorService.Contracts; using Test.GronkulatorService.Model; using Test.TextProcessing; using Test.TextProcessing.Markdown; using Test.TextProcessing.RegularExpressions; using Xamarin; using Xamarin.Forms;
Example: System;Test;*
using System.Collections; using System.Data; using Test.Common; using Test.GronkulatorService.Contracts; using Test.GronkulatorService.Model; using Test.TextProcessing; using Test.TextProcessing.Markdown; using Test.TextProcessing.RegularExpressions; using Microsoft.Windows; using Newtonsoft.Json; using Xamarin; using Xamarin.Forms;
Example: System;Test;**
using System.Collections; using System.Data; using Test.Common; using Test.GronkulatorService.Contracts; using Test.GronkulatorService.Model; using Test.TextProcessing; using Test.TextProcessing.Markdown; using Test.TextProcessing.RegularExpressions; using Microsoft.Windows; using Newtonsoft.Json; using Xamarin; using Xamarin.Forms;
Example: System;**(Test);**
using System.Collections; using System.Data; using Test.Common; using Test.GronkulatorService.Contracts; using Test.GronkulatorService.Model; using Test.TextProcessing; using Test.TextProcessing.Markdown; using Test.TextProcessing.RegularExpressions; using Microsoft.Windows; using Newtonsoft.Json; using Xamarin; using Xamarin.Forms;
Example: System;**(Test,*)
using System.Collections; using System.Data; using Microsoft.Windows; using Newtonsoft.Json; using Test.Common; using Test.GronkulatorService.Contracts; using Test.GronkulatorService.Model; using Test.TextProcessing; using Test.TextProcessing.Markdown; using Test.TextProcessing.RegularExpressions; using Xamarin; using Xamarin.Forms;
Example: System;**(Test,Xamarin,*)
using System.Collections; using System.Data; using Microsoft.Windows; using Newtonsoft.Json; using Test.Common; using Test.GronkulatorService.Contracts; using Test.GronkulatorService.Model; using Test.TextProcessing; using Test.TextProcessing.Markdown; using Test.TextProcessing.RegularExpressions; using Xamarin; using Xamarin.Forms;
Without this, my request is not a duplicate of this, because the functionality I am requesting isn't a part of this feature.
CyrusNajmabadi
commented
4 years ago Without this, my request is not a duplicate of this, because the functionality I am requesting isn't a part of this feature.
FWIW, we're not really certain we'd even do the main feature, because the customer demands and scenarios are so rare. It's extremely unlikely that we'd then do the extra design and implementation work for your case when there would be a suitable workaround for you in the original design.
I don't want to have to repeat that in every project that was previously configured every time I see a new second-level namespace.
I get that. However, from my perspective that's effectively 5 seconds of work on your part for that case. That's not really a compelling thing to make us update the design and add even more complexity to support.
So, I propose that the core syntax be *(list_of_roots), where list_of_roots is a comma-delimited list of namespaces from which children should be separate groups, with meaning that anything unmatched should be included as well. With this, would be shorthand for (*).
this is just vastly complicated, and would be hard to doc and would def drive up the impl costs. I'm not really willing to update the design to handle this one off absent a huge amount of customer demand. The easy alternative is to just add Test.Common; Test.GronkulatorService; Test.TextProcessing to your config.
More time has probably been spent asking for the adjusted feature than would cost to actually make those .editorconfig changes themselves over the lifetime fo the project :)
logiclrd
commented
4 years ago But that work-around forces the position of Test in the resulting formatted list (and so it isn't an effective work-around).
CyrusNajmabadi
commented
4 years ago But that work-around forces the position of Test in the resulting formatted list (and so it isn't an effective work-around).
i don't know what you mean by that. can you clarify? thanks! :)
Also, feel free to join discord.gg/csharp (#roslyn channel), or gitter.im/dotnet/roslyn if you'd like to have more of a realtime discussion on this.
logiclrd
commented
4 years ago If I have some other using statements that sort variously before and after Test, then putting Test.Common, Test.GronkulatorService and Test.TextProcessing into the configuration sequence means that Test will appear at that point in the configuration, and all the other using statements will bunch up after it.
CyrusNajmabadi
commented
4 years ago Yes. That seems like the point :-)
You are specifying where you want certain using groups to go relative to others
logiclrd
commented
4 years ago Right, but I don't want to do that. My feature request is not to specify where I want certain using groups to go relative to others. It's to specify that the second-level namespaces beneath a specified one be treated as separate groups. But my feature request got closed as a duplicate of this one, on the grounds that this one should encapsulate the details of my request. I'm proposing an extension to this one that makes it so that it isn't only specifying the positioning of groups, but also the ability to specify implicit separation into sub-groups.
logiclrd
commented
4 years ago I made a PoC of my proposed rules:
https://github.com/logiclrd/UsingStatementReformatter
..in case that's of any help at all.
CyrusNajmabadi
commented
4 years ago But my feature request got closed as a duplicate of this one
It's a dupe of the general request to expand on grouping and sorting configuration. Your request is noted here. I'm just saying that it's unlikely to be supported given that the design here will let you accomplish what you want with minimal effort. I get you like a design that requires even less effort than that. I'm just saying it's highly unlikely we would support that.
logiclrd
commented
4 years ago Hmm, but what I want is to not have to customize the configuration for every single project I create, and your proposal is that I customize the configuration for every single project I create.
CyrusNajmabadi
commented
4 years ago You can customize in your editorconfig file you then use for your projects. Keeping that file up to date takes a couple of seconds. That is an acceptable workaround for me
logiclrd
commented
4 years ago Every project has its own .editorconfig file, it's stored in the repositories. I'm kind of disappointed by your heavy-handed approach to this:
I've requested a feature that is distinct from anything previously discussed. You've noticed that it has to do with the Remove and Sort Usings feature, similar to this, and declared that it is a duplicate.
So, I've described what my actual feature request is here, and your response is, "Oh, no, that's not what we're doing -- but what you've asked for is still totally a duplicate of this."
I specifically addressed how the feature described here and my request differ -- I am not trying to customize the location of my usings, but rather their format, and I specifically don't want to lock down their location. You haven't responded to this.
I've tried to explain how the feature would impact my workflow, and your response is, "No, the work-around is fine because it matches my workflow."
I work on many projects with many other people, and getting other people to all maintain this file consistently is just not going to happen.
The bottom line is that my feature is not represented by the changes here, and you have so far been resistant to incorporating it here, which means that my feature request is not a duplicate of this one. It is its own feature that you don't want intruding on your design here.
It would be greatly appreciated if you could either reconsider incorporating the feature I'm asking for, or undo the closed-as-duplicate you applied to my feature request now that you've decided my feature isn't part of what you want to implement here.
logiclrd
commented
4 years ago The implementation complexity on this isn't exceedingly high. I'm not sure if you have had the time to take a glance at my PoC. It's not a quick hack, it is carefully engineered and extensively unit tested. I do recognize that clear documentation is more of a challenge.
sharwell
commented
4 years ago @logiclrd Issue #47802 was closed as a duplicate of this issue because the observable end result of both issues is the same. The distinction you have requested relates only to the manner of configuration, and not the resulting behavior once configuration is complete. I believe the behavior you want could be achieved by allowing a .** suffix to a qualified namespace element within the list described by https://github.com/dotnet/roslyn/issues/44052#issuecomment-647828287. For example, Microsoft.** would break up each namespace under Microsoft into groups, as opposed to the current behavior of Microsoft (conceptually equivalent to Microsoft.*) which places all namespaces under Microsoft in the same group.
logiclrd
commented
4 years ago That gets most of the way to what I'm looking for, but I don't want to pin the location of my particular namespaces. I want them to be sorted alphabetically in with the others, just divided into more granular groups. Considering my examples, I don't want Test to always be at the top of the list, above Microsoft and Newtonsoft, nor do I always want it at the end of the list, under Xamarin. But, the only way to put it after Newtonsoft and before Xamarin with the current specification is to explicitly list every single root namespace in alphabetical order. As soon as a new namespace is added, the configuration is out-of-date and the new one ends up dumped wherever * or ** is in the format specification.
My feature request was completely independent of what's described in this issue, in that it didn't intersect with the ordering at all. It only altered how the groupings were made, and that is what I'm looking for. As currently specified, the feature in this issue doesn't even support altering the groupings, but even with the syntax extension you propose, it still can only do it in conjunction with fixing the order.
These aren't the same feature, though the proposal I made and the PoC implementation I did does effectively merge them.
logiclrd
commented
4 years ago I mean, unless I'm completely missing something, and there is a simple specification that can turn this:
using Microsoft.Windows;
using Newtonsoft.Json;
using System.Collections;
using System.Data;
using Test.Common;
using Test.GronkulatorService.Contracts;
using Test.GronkulatorService.Model;
using Test.TextProcessing;
using Test.TextProcessing.Markdown;
using Test.TextProcessing.RegularExpressions;
using Xamarin;
using Xamarin.Forms;...into this:
using System.Collections;
using System.Data;
using Microsoft.Windows;
using Newtonsoft.Json;
using Test.Common;
using Test.GronkulatorService.Contracts;
using Test.GronkulatorService.Model;
using Test.TextProcessing;
using Test.TextProcessing.Markdown;
using Test.TextProcessing.RegularExpressions;
using Xamarin;
using Xamarin.Forms;If that is possible, then please show me how and I will slink away in shame :-D
sharwell
commented
4 years ago @logiclrd Under my candidate proposal in https://github.com/dotnet/roslyn/issues/44052#issuecomment-695350065, the configuration for https://github.com/dotnet/roslyn/issues/44052#issuecomment-695502029 would be this:
System ; Microsoft ; Newtonsoft ; Test.** ; XamarinThe order of these matters (which is a critical design goal because they are not in alphabetical order), but you would not need to add new items for other sub-namespaces of Test.
CyrusNajmabadi
commented
4 years ago Every project has its own .editorconfig file, it's stored in the repositories.
I"m not seeing why that's an issue. No matter what approach we took, you'd need to update the .editorconfig settings.
I'm kind of disappointed by your heavy-handed approach to this
I don't know what to tell you. I'm already leaning heavily against this feature area in general. I felt it would be ok to slightly extend what we do today to at least handle some more common cases we've heard about. However, in no way am i expecting the solution we make (if we provide one) to handle any and all cases people may end up wanting :-/
This issue has been moved from a ticket on Developer Community.
Hi,
In VS options -> Text Editor -> C# -> Advanced -> Using Directives, there is an option "Place System directives first when sorting usings.
Please improve this feature even further, so that the usings are ordered in this priority:
Thank you
Original Comments
Visual Studio Feedback System on 5/4/2020, 02:19 AM:
Thank you for taking the time to provide your suggestion. We will do some preliminary checks to make sure we can proceed further. We'll provide an update once the issue has been triaged by the product team.