Skip to content
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

Removed support for .Net Framework 4.6 #25

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Andi482
Copy link

@Andi482 Andi482 commented Nov 29, 2022

Removed support for .Net Framework 4.6
Upgraded test & benchmark projekt to .net 6.0
Upgraded NuGet packages. Updated CsvHelper benchmark code to the new CsvHelper version
(These changes fixed package downgrade warnings and publish issues with a newer .net 6.0 / EF. Core 7.0 application)

Upgraded test & benchmark projekt to .net 6.0
Upgraded NuGet packages. Updated CsvHelper benchmark code to the new CsvHelper version
var record = reader.Context.Record;
for (int i = 0; i < record.Length; i++)
s = record[i];
IDictionary<string,Object> record = (IDictionary<string, Object>)reader.GetRecord<dynamic>();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a more elegant (and faster) way to read values from the record?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I´m not used to the CsvHelper library. I just tried to make the code working. Should I downgrade the CsvHelper nuget package to the old version, so that the original code is working again?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at CsvHelper Reader class source, I think new code should be (did not test it)

while (reader.Read())
{
    for (int i = 0; i < reader.ColumnCount; i++)
        s = reader[i];
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it´s working with this code.

<PackageReleaseNotes>Added multi-targeting support for .NET 4.6</PackageReleaseNotes>
<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>C:\projects\oss\nlight\src\NLight.snk</AssemblyOriginatorKeyFile>
<PackageReleaseNotes>Increased compatibility to newer .net versions. Removed support for .NET 4.6</PackageReleaseNotes>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please state minimal supported version (.NET 6)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With netstandard2.0 and System.Reactive.Core.5.0.0 we can keep the library backwards compatible to Framework 4.7.2.

@@ -1,22 +1,22 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>netstandard2.0;net46</TargetFrameworks>
<Version>2.1.1</Version>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to target .NET 6 here as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the settings to netstandard, so people who are still using classic .net framework are still able to use this library.

<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>C:\projects\oss\nlight\src\NLight.snk</AssemblyOriginatorKeyFile>
<PackageReleaseNotes>Increased compatibility to newer .net versions. Removed support for .NET 4.6</PackageReleaseNotes>
<!--<SignAssembly>true</SignAssembly>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please uncomment. I will publish to Nuget once PR merged

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@slorion
Copy link
Owner

slorion commented Nov 30, 2022

Hello, thank you for this PR, very much appreciated and I'm glad that my library is still being used after all those years ;-)

@slorion
Copy link
Owner

slorion commented Dec 1, 2022

Based on your comments in the code, I am not sure I understand what is the minimum change required to remove package downgrade warning when using .NET 6? Taking a step back, it would probably make sense to refresh the library by targeting .NET 6 since it merges netcoreapp and netstandard.

@Andi482
Copy link
Author

Andi482 commented Dec 1, 2022

Based on your comments in the code, I am not sure I understand what is the minimum change required to remove package downgrade warning when using .NET 6? Taking a step back, it would probably make sense to refresh the library by targeting .NET 6 since it merges netcoreapp and netstandard.

I had the following downgrade warning with the old library during publish:

Detected package downgrade: System.IO.FileSystem from 4.3.0 to 4.0.1. Reference the package directly from the project to select a different version.
32> MyProject -> NLight 2.1.1 -> System.Reactive.Core 3.1.1 -> System.Reactive.Interfaces 3.1.1 -> NETStandard.Library 1.6.0 -> System.Net.Sockets 4.1.0 -> runtime.win.System.Net.Sockets 4.3.0 -> System.IO.FileSystem (>= 4.3.0)
32> MyProject -> NLight 2.1.1 -> System.Reactive.Core 3.1.1 -> System.Reactive.Interfaces 3.1.1 -> NETStandard.Library 1.6.0 -> System.IO.FileSystem (>= 4.0.1)

Upgrading the library to System.Reactive.Core 5.0.0 solved this problem. But 5.0.0 is not longer compatible with net4.6, so I removed this target framework. But it is compatible with netstandard 2.0 and can also be used in old .net 4.7.2. projekts (System.Reactive.Core is also compatible to 4.7.2, according to the information on the nuget page for this package)

I wanted to make sure, that the library keeps compatible to as many projects as possible, and people "stucked" to classic framework can still use this library.

I could also fix it by manually adding System.IO.FileSystem 4.3.0 to MyProject, but I think it is much easier, when it work´s out of the box.

<Copyright>Copyright (c) 2009 Sébastien Lorion</Copyright>
<Authors>Sébastien Lorion</Authors>
<Company />
<Description>Toolbox for .NET projects</Description>
<PackageProjectUrl>https://github.com/slorion/nlight</PackageProjectUrl>
<PackageTags>io,parser,csv,delimited,transaction,reactive,tree</PackageTags>
<PackageReleaseNotes>Added multi-targeting support for .NET 4.6</PackageReleaseNotes>
<PackageReleaseNotes>Increased compatibility to newer .net versions. Minimum supported .NET Version is 4.7.2 (.NET Standard 2.0)</PackageReleaseNotes>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change to "Increased compatibility with newer .NET versions. Updated Reactive dependency to 5.0. NLight targets .NET Standard 2.0 (.NET Framework 4.6.1+, .NET Core 2.0+)."

See https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0#net-5-and-net-standard for compatibility matrix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done.

@slorion
Copy link
Owner

slorion commented Dec 1, 2022

Thanks, I get it now. I will just have a quick look how to update the benchmark code for CsvHelper to avoid using dynamic.

@slorion
Copy link
Owner

slorion commented Dec 7, 2022

Thank you for update, I will be able to have final look and merge next week.

@Andi482
Copy link
Author

Andi482 commented May 23, 2023

Hi, do i need to do anything else? Or can the code be merged?

@slorion
Copy link
Owner

slorion commented May 29, 2023

Sorry for delay, I had to prioritize work, but hopefully will get around this by end of June.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants