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

Target only supported frameworks #76

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

Conversation

silkfire
Copy link

@silkfire silkfire commented Jul 16, 2021

Summary:

I'm of the opinion that a modern library should only target framework versions that are currently supported. I'm noticing that FlatFiles targets .NET Core 1.x which is no longer supported. There are also dependencies that are not required for the library to function included by default for all TFMs.

Breakdown:

  • To streamline the framework targets I believe that three targets are sufficient: .NET Framework 4.6.2, .NET Standard 2.0 and .NET Standard 2.1.
  • Target .NET Framework 4.6.2 instead of 4.5.1 because anything below is 4.6.2 is considered insecure: https://dotnet.microsoft.com/platform/support/policy/dotnet-framework. Additionally, support for .NET Framework 4.5.1 ended on January 12, 2016.
  • IAsyncEnumerable can be fully supported by .NET Framework 4.6.2+ and .NET Standard 2.0 by using the official dependency Microsoft.Bcl.AsyncInterfaces and enabling C# 8.0 language features.
  • 32 unit tests are failing already before this PR, perhaps you're already aware of this?

References:

https://docs.microsoft.com/en-us/lifecycle/products/microsoft-net-framework
https://dotnet.microsoft.com/platform/support/policy/dotnet-core
https://docs.microsoft.com/en-us/dotnet/standard/net-standard
https://devblogs.microsoft.com/dotnet/net-framework-4-5-2-4-6-4-6-1-will-reach-end-of-support-on-april-26-2022/
https://support.microsoft.com/en-us/topic/-net-framework-retiring-sha-1-content-9750f20d-a9ef-4d43-853f-2075f0a9d7da

Check list:

  • [➖] If this is a new feature, was a feature request created for it and discussed with the project coordinators?
  • [❌] After making your change, have all unit tests been updated and ran successfully?
  • [➖] Did you write new tests to cover any new features?
  • [✔] Did you clean your code to use best coding practices?
  • [✔] Are all changes relevant/necessary for your change?
  • [➖] Did you run the performance project before and after to verify your changes did not degrade performance?

@jehugaleahsa
Copy link
Owner

@silkfire I am in the middle of incorporating a lot of improvements. Some of what I am working on can be seen here: https://github.com/jehugaleahsa/FlatFiles/wiki/What-I'd-want-to-see-in-version-5.0

It's possible I accidentally pushed to master without all my tests passing, although that doesn't seem right. It makes me wonder if our environments are different somehow. I know in the past some folks with different default locales reported having issues with tests assuming "en-us", so let me know if that's the sort of issue you're seeing. I just fixed a bunch of tests recently so perhaps I'll recognize your failures and realize I did push a bad build.

I'm hoping over the next couple of months to hit each of those bullet points listed on that wiki. I am trying to prioritize changes I can make in the 4.x release without breaking changes. I plan on creating a 5.0.0 version that includes breaking changes - I will probably reevaluate minimum platform support as part of this. I will take a careful look at your PR to see what I can incorporate. Mostly, I would love to eliminate the vast majority of the conditional compilation stuff I have today. I will take a look at your IAsyncEnumerable feedback and see if I can make that available for even older platforms for 4.x.

I definitely will heed your advice about making .NET framework 4.6 the minimum. I may or may not support .NET Standard 2.0. I get a warning that .NET Core 3.0 is no longer supported, so I will see about bumping the minimum version for .NET Core, as well. I wasn't aware that I supported .NET Core 1.0... so I will look into that. In fact, I remember originally postponing my adoption of .NET Core 1.0 because of the issues I encountered.

Right now, I am in the middle of two changes: moving each top-level interface/class into its own file and enabling/cleaning up nullable reference compilation warnings. Once I complete that, I will see if I can incorporate any of your changes at that point. I will definitely push out to master at that point, too, so we can see if all the tests pass at that point on your machine.

@silkfire
Copy link
Author

Thanks for your quick response. I wasn't aware of the numerous things you had planned for this amazing library!
That list seems quite ambitious, especially the pipelines implementation, but I really like that you're actively maintaining FlatFiles and providing some high quality updates regularly :)

Feel free to either delete my PR or keep it open until the framework target changes have been implemented 👍🏻

@silkfire silkfire force-pushed the target-supported-tfms branch from efa978a to 26dcffc Compare July 19, 2021 14:02
@jehugaleahsa
Copy link
Owner

jehugaleahsa commented Jul 19, 2021

I will keep your PR open. I will probably just steal what I need and then close it. I agree that a lot of what is in that list is ambitious. In fact, I spent a good part of my weekend adding nullable reference support and breaking out each top-level interface/class into its own file. I will probably tackle rewriting the code generation logic next. At that point I should be ready to do a 4.x release. Then I will start focusing on breaking 5.x changes. I would like to avoid going months without a deployment, if possible, especially if any bug reports come through in the meantime. Cool thing: as part of adding nullable reference checking, I found a way to simplify my reader logic and made async operations faster as a result. Yay!

@silkfire
Copy link
Author

Performance improvements are always a welcome thing. Great work, @jehugaleahsa!

@silkfire silkfire force-pushed the target-supported-tfms branch from 26dcffc to 85935b5 Compare August 14, 2021 21:34
@CoffeeCodes
Copy link

I will keep your PR open. I will probably just steal what I need and then close it. I agree that a lot of what is in that list is ambitious. In fact, I spent a good part of my weekend adding nullable reference support and breaking out each top-level interface/class into its own file. I will probably tackle rewriting the code generation logic next. At that point I should be ready to do a 4.x release. Then I will start focusing on breaking 5.x changes. I would like to avoid going months without a deployment, if possible, especially if any bug reports come through in the meantime. Cool thing: as part of adding nullable reference checking, I found a way to simplify my reader logic and made async operations faster as a result. Yay!

Hello, are there any news on this one? Your plans sound great, but the last release was over 2 years ago :-(

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.

3 participants