Skip to content

Conversation

BorisDog
Copy link
Contributor

No description provided.

@BorisDog BorisDog requested a review from a team as a code owner September 11, 2024 01:58
@BorisDog BorisDog requested review from sanych-sun, JamesKovacs and rstam and removed request for a team and sanych-sun September 11, 2024 01:58
@@ -84,59 +84,6 @@ public static bool CanTranslate(MethodCallExpression expression)
return true;
}

#if NETSTANDARD2_0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rstam requesting review just for the LINQ changes. Should be straightforward, but just in case.

Copy link
Contributor

@rstam rstam left a comment

Choose a reason for hiding this comment

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

LGTM

@BorisDog BorisDog requested review from sanych-sun and removed request for JamesKovacs September 11, 2024 17:44
Copy link
Member

@sanych-sun sanych-sun left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -304,7 +299,6 @@ Task("TestCsfleWithMongocryptd")
RunTests(buildConfig, testProject, filter: "Category=\"CSFLE\""));

Task("TestCsfleWithMongocryptdNet472").IsDependentOn("TestCsfleWithMongocryptd");
Task("TestCsfleWithMongocryptdNetStandard20").IsDependentOn("TestCsfleWithMongocryptd");
Task("TestCsfleWithMongocryptdNetStandard21").IsDependentOn("TestCsfleWithMongocryptd");
Task("TestCsfleWithMongocryptdNet60").IsDependentOn("TestCsfleWithMongocryptd");
Copy link
Member

Choose a reason for hiding this comment

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

Also there is reference to netstandard2.0 at line 655 in Setup<BuildConfig> method that might be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@BorisDog BorisDog merged commit 1fc7c7c into mongodb:main Sep 11, 2024
12 of 13 checks passed
@BorisDog BorisDog deleted the csharp4916 branch September 11, 2024 18:38
@eerhardt
Copy link

@BorisDog - there's no description/reasoning for this change here. Was the reason discussed elsewhere (like in JIRA)?

@BorisDog
Copy link
Contributor Author

Hi @eerhardt,
The research and discussion for this change was tracked internally.
For this change we've considered multiple factors like

  1. .NET Core 2.2 is EOL for 5 years
  2. Our usage numbers for .netstandard 2.0
  3. Maintainability of our code and our testing after adding newer TFMs (we've added .NET 6.0)

@terrajobst
Copy link

You still support .NET Framework, right?

The downside of dropping .NET Standard 2.0 for your consumers is that if they want/need to support both .NET Framework and .NET Core, then they need to multi-target now (i.e. build for both). The benefit of .NET Standard 2.0 is that most consumers don't need to do that and get away with building for a single framework and get a single binary.

FWIW, everything we ship that needs to support both still targets .NET Standard 2.0.

@BorisDog
Copy link
Contributor Author

Yes, the driver targets net472 as well.
That's a good point, but unfortunately for such users our guideline is to multi-target their apps due to some nuances in Random implementation, which caused some bugs in the driver.
Additional contributing factor for this change, was our internal security requirements which made it challenging to use .NET Standard 2.0.

@terrajobst
Copy link

@BorisDog

Additional contributing factor for this change, was our internal security requirements which made it challenging to use .NET Standard 2.0.

Interesting. Would you mind sharing these concerns? If you don't want to do this publicly, you can email me immol at microsoft dot com.

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.

5 participants