search

testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.
https://dotnet.testcontainers.org
MIT License
3.77k stars 271 forks source link

chore: Invert #if NETSTANDARD* conditional compilation conditions #1079

Closed 0xced closed 9 months ago

0xced commented 9 months ago

While conceptually .NET 5, 6, 7, 8 are greater than .NET Standard 2.1 it does not apply to the NETSTANDARD2_1_OR_GREATER macro.

So it's safer to conditionally compile on NETSTANDARD2_0 and assume that the #else branch targets a newer framework where the new bits are available in order not to use the old code path on the newest frameworks.

netlify[bot] commented 9 months ago

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit ed07855e02c463d6472c30fd129974a9bad62ea7
Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/658c595ea20d620008de6e88
Deploy Preview https://deploy-preview-1079--testcontainers-dotnet.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

0xced commented 9 months ago

Hmm, I am curious, how did you run into this issue?

Sorry I should have been more clear in my description. This is currently not an issue. This pull request is solely future-proofing the conditional compilation so that if a new target framework is added in the future (I have a plan 😉) then the correct (newest) code will be compiled. This stuff is easy to get wrong, I already fixed it in https://github.com/spectreconsole/spectre.console/pull/563 too.

If you consume the NuGet dependency, for example, in a net8.0 project, it will use the netstandard2.1 version.

Yes, absolutely! This is not an issue for consumers of the Testcontainers packages.