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

Improve Code Quality #795

Merged
merged 17 commits into from
Mar 15, 2024
Merged

Improve Code Quality #795

merged 17 commits into from
Mar 15, 2024

Conversation

iamcarbon
Copy link
Collaborator

@iamcarbon iamcarbon commented Mar 14, 2024

This PR:

  • Drops unsupported .NET frameworks. All .NET frameworks < 4.6.2 have gone out of support.
    https://learn.microsoft.com/en-us/lifecycle/products/microsoft-net-framework
  • Cross targets .NET8.0
  • Updates LangVersion to 12
  • Utilizes statically initialized data (allocation free)
  • Add System.Memory dependency to utilize ReadOnlySpan and utf-8 literals
  • Add Microsoft.Bcl.HashCode dependency and utilizes HashCode.Combine
  • Replaces EmptyArray with Array.Empty
  • Add various missing braces
  • Utilized vectorized SequenceEqual method
  • Uses BinaryPrimitives to read big endian integers

@iamcarbon
Copy link
Collaborator Author

iamcarbon commented Mar 14, 2024

@ EliotJones Ready for review / feedback.

@BobLd
Copy link
Collaborator

BobLd commented Mar 14, 2024

@EliotJones I did a quick review and it looks good to me.

I think it's about time we drop out of support framework, so this is a great opportunity to do so

@Numpsy
Copy link
Contributor

Numpsy commented Mar 14, 2024

Purely on the subject of TFMs

  1. I think adding a .NET 4.7.1 target would remove the System.ValueTuple NuGet dependency for consumers of that version (e.g. Serilog targets 4.6.2 and 4.7.1 for that reason)
  2. If the main lib is built as both .NET 6.0 and 8.0 then maybe the unit tests should be as well?

@iamcarbon
Copy link
Collaborator Author

With net6.0 going out of support in November (of this year), what do we think about bumping the testing target to .NET8.0 as well? .NET8.0 will be supported until November 2026.

Purely on the subject of TFMs

  1. I think adding a .NET 4.7.1 target would remove the System.ValueTuple NuGet dependency for consumers of that version (e.g. Serilog targets 4.6.2 and 4.7.1 for that reason)
  2. If the main lib is built as both .NET 6.0 and 8.0 then maybe the unit tests should be as well?
  • Agreed. I bumped the net47 target to net471 (matching Serilog). The System.ValueTuple dependency is only needed on net462 now.
  • I added net8.0 to the testing targets. We should consider dropping the net6.0 test target in November when it goes out of support, to reduce build overhead.

@BobLd
Copy link
Collaborator

BobLd commented Mar 14, 2024

@iamcarbon I think it's safe to drop the net6.0 test target for the net8.0

Also, the tests are now failing... might be related, I think there's an issue in the yaml change: seems like it cannot target 2 different dotnet versions (only target net8.0 then?)

As a side note, I think it's better to not make this PR too much bigger for ease of review. The great job you did with span opens a lot of optimisation which can be done in later PRs

I'll let @EliotJones give his opinion on obsolete framework version. I need to review the changes more thoroughly but I'd be happy to merge the PR as it is I think

Copy link
Member

@EliotJones EliotJones left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

@BobLd BobLd merged commit d413d24 into UglyToad:master Mar 15, 2024
1 check passed
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.

4 participants