search

nunit / NUnit.System.Linq

Partial implementation of System.Linq for use with NUnit's .NET 2.0 builds
MIT License
1 stars 4 forks source link

Make methods internal #12

Closed ChrisMaddock closed 5 years ago

ChrisMaddock commented 7 years ago

The current implementation allows the LINQ methods to leak out into user code. This can cause collisions, such as in https://github.com/nunit/nunit-console/pull/152 - when the methods implemented here are available from multiple different sources.

We should make the LINQ methods should just be available for use in the framework assembly, and not exposed to downstream users. This could be done with InternalsVisibleTo

CharliePoole commented 7 years ago

Isn't this a little overkill for the problem? Users can't see anything in NUnit.System.Linq unless they reference it. Certainly, we have a problem with our own tests but that can be fixed (I think) by making just that single attribute internal and without using InternalsVisibleTo... Unless I'm missing something.

ChrisMaddock commented 7 years ago

I wasn't sure if the attribute was exposed for use by assemblies referencing the NUnit.System.Linq package? If that's not the case, then sure.

Users can't see anything in NUnit.System.Linq unless they reference it

Adding the 2.0 nunit.framework nuget package currently automatically adds a reference to NUnit.System.Linq, which is why I thought it may affect more than just us. Should that not be the case, then?

CharliePoole commented 7 years ago

I can see it's a bit more complicated. I'd suggest making one change at a time.

The reference added by nuget is just default nuget behavior. I don't think it's needed but it should be tested. Sometimes C# wants you to reference a second level assembly. If you can remove the reference from our own tests and rebuild we're probably safe.

NN--- commented 5 years ago

In .NET 2.0 project I have LinqBridge reference and NUnit. It causes collision in Enumerable extensions.

jnm2 commented 5 years ago

The minute @nunit/framework-team is okay with this, I'd love to drop net20 from the framework and archive this repository.

@NN--- Why not target just your test csprojs to .NET Framework 3.5, leaving everything else at .NET Framework 2.0? That would resolve the problem you're facing.

NN--- commented 5 years ago

I prefer to not use 3.5 for tests. It would be much better to use internal.

There is workaround using Aliases: https://github.com/NuGet/Home/issues/4989

jnm2 commented 5 years ago

I prefer to not use 3.5 for tests. It would be much better to use internal.

Can you elaborate on why, please? We will need to gauge the consequences of one day dropping support.

NN--- commented 5 years ago

If I have library for .NET 2.0 , then tests should be .NET 2.0 . I don't think there is anyone who still uses 2.0. The only issue is that many libraries have support for all .NET versions starting from 2.0.

jnm2 commented 5 years ago

Do you know of any cases where a project compiled as .NET 3.5 ever runs differently than the same project compiled as 2.0?

I don't think anyone still uses net20 either, and supporting it has been expensive for us time-wise. Most libraries I know of only support net45 or net46, and many recently are even making net472 a minimum requirement based on recommendations from Immo Landwerth.

Maybe if NUnit stops supporting net20, it could give these libraries you mention permission in a sense to stop supporting net20?

NN--- commented 5 years ago

If project is using reflection it behaves differently. I think that your point is good. Stopping support for .NET 2.0 will eventually cause libraries to stop providing .NET 2.0 build.

jnm2 commented 5 years ago

I can't think of any reflection scenarios that would behave differently other than new types and new members being available.

Sounds good then. My logic is that if a net20 project is still in development, it should upgrade to a newer .NET Framework and fix any bugs due to not thinking ahead when using reflection (which I expect in practice to be extremely rare). If a net20 project is not still in development, it shouldn't have a reason to upgrade to newer NUnit framework or runners.

rprouse commented 5 years ago

@NN--- is your .NET 2.0 library open source? If so, could you give us a link and also let us know which downstream libraries that you know of that still support .NET 2.0? We are seriously considering dropping 2.0 support in NUnit, so we want to understand the impact. .NET 2.0 has been out of support for several years now, so we are trying to understand who is still using it and why.

NN--- commented 5 years ago

For instance this library supports .NET 2.0: https://github.com/dahall/Vanara

NN--- commented 5 years ago

Another one: https://github.com/AArnott/pinvoke

rprouse commented 5 years ago

I only did a quick look, but Vanara's unit tests target net471 and pinvoke's tests target net46, so we would be fine there.

jnm2 commented 5 years ago

As of 3.12, the framework no longer supports .NET Framework 2.0 or brings NUnit.System.Linq into users' projects. Currently I don't think we would plan to release a 3.11 patch that supports .NET Framework 2.0 without bringing NUnit.System.Linq into users' projects.