-
-
Notifications
You must be signed in to change notification settings - Fork 423
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.
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
Create an analyser package that looks for collection initialiser usage with immutable language-ext types (the results of the "Add" methods are discarded, which is bad) #868
base: main
Are you sure you want to change the base?
Conversation
…e with immutable language-ext types (the results of the "Add" methods are discarded, which is bad)
@ProductiveRage Thanks for this Dan, the project naming is inconsistent with the other projects, so |
I'll look to change those name spaces for consistency. In relation to the dependencies - analysers run within the context of Visual Studio and not the solution that you have open. So you can add as many analysers as you want, with whatever crazy tree of dependencies that they may have and those dependencies will not work their way into your solution code. Does that make sense? I think it's a non-issue but I might be getting my wires crossed. (It's worth having a look at the way that analysers are installed - via some .ps files that add them into the project in a special manner and not as a standard project / assembly reference whose dependencies would be pulled in). |
With the code-gen stuff you can declare dependencies that don't appear in the final build, which may be the same thing here. I've never built a Roslyn analyzer, so I'd need to get my head around the implications. The last thing I need is ImmutableCollections embedded in the Core lib. I'll give you a shout on Teams later in the week and we can go over it. |
Sounds good. And I'm totally behind ImmutableCollections not getting into the library which already has its own immutable collections! |
…analyser only and doesn't pull the analyser's dependencies into the NuGet-ingesting project
So you can add as many analysers as you want, with whatever crazy tree of dependencies that they may have and those dependencies will not work their way into your solution code. Actually when user adds analyzer to solution it might gets added to application output, based on settings used with analyzers package |
@Lonli-Lokli - I think that's correct; that it's possible to have analyser assemblies appear in the build output of the application that has the analyser loaded into one (or more) of its projects however I don't believe that I've seen it before myself and it should certainly be something that can be prevented with the right options. I've pushed another couple of changes to illustrate it, after testing locally - we'll see if it works properly for everyone! |
it should certainly be something that can be prevented with the right options. Thats right, I mean that it's better to include correct way of using analyzer in README. By correct I mean this steps
|
Agreed. Hopefully, if the analyser is included in the main library (like the xunit analysers are) then no Readme will even necessary! We'll just test it out thoroughly and then it should be a seamless experience for anyone using the library! |
I realised that I didn't add a note here specifically to say that I tested as @Lonli-Lokli suggested and the build output of a project that includes the analyser is the same as one that doesn't - so the analyser dependencies do not end up in the build output of the project that includes the analyser, which is the result that we wanted to see (ie. no README or other setup details would be necessary to include this analyser functionality in this library). |
This isn't quite a complete change because it might make sense to include the analysers as a dependency of the main package, so that if you install language-ext then you get the analysers as well (like I believe xUnit does).
Also, I wasn't sure if you would want to tweak the package name or the namespaces - I've tried to be consistent with what is used in the rest of the solution but I may have overlooked something.
The idea behind the single analyser that is included is that it detects when a language-ext type is "populated" by a collection initialiser ("populated" in quotes because it won't actually work) and records an error to inform the code writer:
It only does this work for language-ext types, it doesn't prevent the collection initialiser from being used with the standard mutable BCL List type, for example.
Relates to this C# proposal that you raised: Opt-out attribute for collection initialization.