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

.NET Standard 2.0 compatibility #16

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

szakatsa
Copy link

Adds .NET Standard 2.0 compatibility

Copy link
Owner

@aevitas aevitas left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I'll need to read up on these monikers and support matrices for .NET standard and the individual framework versions.

Perhaps it would be nice to add tests for the various versions. Right now we only have 8.0, but if there's a way to ensure backward compatibility with this change through tests, that'd be great.

// int processId = s_processId ??= Process.GetCurrentProcess().Id & ProcessIdMask;
if (s_processId is null)
s_processId = Process.GetCurrentProcess().Id & ProcessIdMask;
int processId = s_processId.Value;
Copy link
Owner

Choose a reason for hiding this comment

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

This is the exact same with extra steps, I'd much prefer to keep the old code.

Copy link
Author

Choose a reason for hiding this comment

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

You are absolutely right, and your single line of code is ingenious, but this is the only change not compatible with the old language version, which is a requirement for supporting .NET Standard 2.0

Copy link
Owner

Choose a reason for hiding this comment

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

I see. Even then there is a more succinct way of doing it, something along the lines of:

s_processId.HasValue ? s_processId.Value : (s_processId = Process.GetCurrentProcess().Id & ProcessIdMask)

Now that I'm reading this though, part of me thinks we shouldn't shift the mask into s_processId at all, but that's something for later.

Copy link
Author

Choose a reason for hiding this comment

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

I know it is three lines instead of one, but this was a clear way of doing it. I don't insist on using the exact lines, feel free to replace it with anything compatible with good old C# 7.3 (the latest language supported by .NET Standard 2.0)

Small changes in the code to build on lower (7.3) C# versions
@szakatsa
Copy link
Author

I updated the tests to reflect all .NET Standard 2.0 compatible framework tests

@aevitas
Copy link
Owner

aevitas commented Jul 11, 2024

I updated the tests to reflect all .NET Standard 2.0 compatible framework tests

Nice one. I think there should be multiple runners instead though, so we target netstandards2.0 and 2.1, and then have several runners on GitHub actions that run tests each on a different platform, e.g. net8.0, net7.0, etc. They'll all be compatible, but will have different underlying runtimes. I need some time to figure out how to properly do this.

Out of curiosity, did you open this PR because you need support for traditional .NET Framework applications?

@szakatsa
Copy link
Author

I updated the tests to reflect all .NET Standard 2.0 compatible framework tests

Nice one. I think there should be multiple runners instead though, so we target netstandards2.0 and 2.1, and then have several runners on GitHub actions that run tests each on a different platform, e.g. net8.0, net7.0, etc. They'll all be compatible, but will have different underlying runtimes. I need some time to figure out how to properly do this.

Out of curiosity, did you open this PR because you need support for traditional .NET Framework applications?

The tests must target actual frameworks, not .net standard. This is why the test project contains all the framework versions compatible with .NET Standard. One runner can run all tests (which it does if you issue the command) as long as it contains all the necessary frameworks. You just have to adapt the runner to contain all frameworks.

Yes, I'm working with a sophisticated application which accepts extension modules in a form of dll files implementing an interface, so I need .net 4.7.1 support.

@aevitas
Copy link
Owner

aevitas commented Jul 18, 2024

I updated the tests to reflect all .NET Standard 2.0 compatible framework tests

Nice one. I think there should be multiple runners instead though, so we target netstandards2.0 and 2.1, and then have several runners on GitHub actions that run tests each on a different platform, e.g. net8.0, net7.0, etc. They'll all be compatible, but will have different underlying runtimes. I need some time to figure out how to properly do this.
Out of curiosity, did you open this PR because you need support for traditional .NET Framework applications?

The tests must target actual frameworks, not .net standard. This is why the test project contains all the framework versions compatible with .NET Standard. One runner can run all tests (which it does if you issue the command) as long as it contains all the necessary frameworks. You just have to adapt the runner to contain all frameworks.

Yes, I'm working with a sophisticated application which accepts extension modules in a form of dll files implementing an interface, so I need .net 4.7.1 support.

I see. In that case, what do you say we support netstandard2.0 (and perhaps .NET 8 as it's the latest) in the FlakeId library, and have the test runners target .NET 8.0 and .NET Framework 4.7.2 or whichever is the latest LTS version of the Framework? According to the .NET Standard matrix, that should give us the widest range of supported frameworks without having to target individual ones.

Once we get the targeting right and tests are passing without warnings, I'd be happy to merge this and publish a new release.

@aevitas
Copy link
Owner

aevitas commented Jul 18, 2024

I'll merge this and make the last few changes before pushing a new release. Thanks for contributing!

@aevitas aevitas merged commit 14259d4 into aevitas:master Jul 18, 2024
1 check failed
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