ljnath / RandomString4Net

.NET library to generate N random strings of M length from various categories
MIT License
9 stars 2 forks source link

Remove outdated and unsupported .NET frameworks and update all projects to SDK format #5

Open jshergal opened 1 month ago

jshergal commented 1 month ago

Currently there are several unsupported targets in the project files. This can all be consolidated into fewer targets by targeting: netstandard2.0; netstandard2.1; net6.0 and net8.0.

.NET Core 1.0 - 3.1, .NET 5 and .NET 7 are all out of support. (https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core)

.NET Framework 2.0 and 3.5 are not testable using the latest MSTest and since there is already a release that supports those targets, it seems like it would be okay to drop those.

jshergal commented 1 month ago

Hi @ljnath. I read your comment on the skipped PR, but could you clarify how much backward compatibility you want to keep? It would be easy to add back in the .net20 and .net35 moniker targets. I removed them in the PR since they were not testable. I would be happy to work on adding an additional test project that would test those frameworks, as I imagine it is possible.

I guess my question comes back more to how far back to support. With the change to .netstandard2.0, 2.1 and then .NET 6 and 8, every framework is still supported with the exceptions of .NET Framework 2.0 and 3.5, which I can't imagine are still being used heavily. I suspect by now that most have moved over at least to .NET Framework 4.

jshergal commented 1 month ago

Hi @ljnath. I did a little more reading to try to narrow down the issue with the CI build and I am fairly certain that it is not possible to use the CI build using GitHub Actions (without self hosting of course) that will support a build targeting .NET Framework 2.0 and .NET Framework 3.5. If you take a look here:

https://github.com/actions/runner-images/blob/main/images/windows/Windows2019-Readme.md

You can see that .NET Framework 3.5 isn't listed. It may be enabled and so the build may still work. I suppose you would have to test things out to no for sure if it is there or not, but that might be some motivation to consider actually dropping the support for the older targets and just go with .netstandard 2.0 & 2.1, along with .NET 6.0 & 8.0 (and shortly 9.0). That would still give very broad support.

jshergal commented 1 month ago

Hi @ljnath, I did manage to add in the reference assemblies so that a build targeting .NET Framework 2.0 and 3.5 will work. As I mentioned earlier and you can see in my PR, I did manage to create an NUnit test project that will run .NET 3.5 tests, but we cannot update that project to the latest version of NUnit as the latest version only supports projects going back to .NET 4.6.

All of that again leads me to a conclusion that maybe dropping support for .NET Framework 2.0 and 3.5 would be okay. If we set the main project to target .NET Standard 2.0 and 2.1, then anyone who is at least on .NET Framework 4.6.1 would be able to run it.

My recommendation is that you allow the PR that I have made that updates the project formats as well as the new test project, etc. We can also then update the github action YAML file. After that is done, we could go ahead and add in the PR to fix the issue related to the entropy from the random seed. Then once all of that is done, you could make a tag and a new release that will still support the older frameworks.

Once things are tagged, then we could move everything to .NET Standard 2.0 & 2.1 and .NET 6.0 and 8.0. I could then consolidate the testing into a single test project. I would then add in a fix (and associated unit tests) for the forceUnique bug I found.

What do you think about that? Please let me know, as I need to make a decision on whether I will just take the fork I have created from this repo and run with it and create a separate NuGet package (which I don't want to do), or if we could work together to improve the current package. I have plans for several other PRs and updates to the code related to a book I am working on and the code base here is a great example case.

ljnath commented 1 month ago

Hello @jshergal , I have shared my view on this PR https://github.com/ljnath/RandomString4Net/pull/6#issuecomment-2398852171.

Please share your perspective as well. I’m confident we can find common ground.

jshergal commented 1 month ago

Hi @ljnath, I have already forked the project and created my own NuGet package. There are several performance improvements I want to make and trying to maintain support back to .NET Framework 2.0 and 3.5 was going to make the improvements more difficult. I just don't feel it to be worth it at this point in time. I will shortly drop support for .NET Standard 2.1 in my own library since I will be supporting .NET 6.0, 8.0 and 9.0. I will keep support for .NET Standard 2.0 since that supports .NET Framework 4.6, 4.7 and 4.8, so I do want to maintain that support, but since .NET Core 1-3.1, .NET 5.0, etc are now officially out of support, by targeting just .NET Standard 2.0 along with .NET 6.0, 8.0 and 9.0, I will be covering nearly all officially supported .NET environments. I am trying to avoid having to litter the code with #if #endif statements to deal with the different targets. I will still have to have some, but this will allow me to minimize those.

Forking also gave me more freedom to revamp some, in my opinion, pain points as well as adjust the coding style to more modern .NET conventions (use of var, capitalization, etc). You are welcome to take a look: https://github.com/jshergal/StuceSoftware.RandomStringGenerator

I switched over to the xUnit testing library with FluentAssertions. I also revamped the enum to be a flags style so that rather than having a huge collection of enums, there are only 4 and they can be combined to select the character classes. In my opinion, it makes the character class selection cleaner and more readable. It also has the added benefit of simplifying the GetString() method.