morelinq / MoreLINQ

Extensions to LINQ to Objects
https://morelinq.github.io/
Apache License 2.0
3.63k stars 409 forks source link

Enable `ImplicitUsings` #1038

Closed viceroypenguin closed 5 months ago

viceroypenguin commented 7 months ago

This PR enables ImplicitUsings on the MoreLinq project and removes (now-)unnecessary usings on relevant files. This is a pre-requisite for #947

Open questions:

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a0f72ab) 92.50% compared to head (4248492) 92.50%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1038 +/- ## ======================================= Coverage 92.50% 92.50% ======================================= Files 112 112 Lines 3404 3404 Branches 1056 1056 ======================================= Hits 3149 3149 Misses 189 189 Partials 66 66 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

viceroypenguin commented 7 months ago

NB: All of the CodeFactor issues are pre-existing and should be addressed separately.

viceroypenguin commented 7 months ago

This is a pre-requisite for #947

Are you sure? How? I don't think implicit usings are a great idea, especially because they make it difficult to understand the code base and exclude embedding scenarios. They may have their place in some projects, but don't think this is one of those.

Noted in this comment.

viceroypenguin commented 7 months ago

I have a few more minutes to respond, so here goes:

Problems with ImplicitUsings

exclude embedding scenarios.

I'll disagree with this for two reasons:

  1. Embedding scenarios were common in the late '00s and early '10s, before the rise of nuget. It was a necessary way to include code, because using randomly built DLLs across the internet was unsafe and untrusted. Now that nuget packages are commonplace, people are not embedding code anymore, and we should not be concerned with whether people are embedding MoreLinq code anymore. MoreLinq is clearly popular enough that people are referencing the package directly, so it is unlikely that people are doing copy/paste from MoreLinq into other files.
  2. A majority of projects now have ImplicitUsings enabled; this is because all of the default templates since net6+ have it enabled in the .csproj, for the convenience of top-level programs. As such, if anyone were to copy files from MoreLinq into their projects, they are more likely to be working with the default ImplicitUsings.

it difficult to understand the code base

What does removing using System; make harder to understand? I think most developers can agree that IDisposable and ArgumentNullException come from the System namespace, and IList<> comes from the System.Collection.Generic namespace. And if they don't know that, then a) are they working with the MoreLinq codebase in the first place, and b) do they care? The default implicit usings contain the following namespaces:

System
System.Collections.Generic
System.Linq
System.Text
System.Threading.Tasks

Which classes from these namespaces are difficult to understand where they come from or how they are used? And which of these classes are likely to overlap with any code written by us or other developers?

Why ImplicitUsings is necessary

That all said, the problem with not using ImplicitUsings or global using System; is that we cannot use a global using for ArgumentNullException if using System; is present at the top of the file. The file and/or namespace usings have higher priority than the global usings. As such, if each file has using System;, then each file must separately have the following code at the top fo the file:

namespace MoreLinq
{
   using System;

#if !NET6_0_OR_GREATER
   using ArgumentNullException = MoreLinq.Exceptions.ArgumentNullException;
#endif
#if !NET8_0_OR_GREATER
   using ArgumentOutOfRangeException = MoreLinq.Exceptions.ArgumentOutOfRangeException;
#endif

   // code
}

I think that this is untenable to have across the project.

Options:

  1. As written, with the open questions listed above
  2. Add a GlobalUsings.cs file with the necessary global usings, for System and for the aliases.
  3. use the two #ifdefs in a majority of files.