-
Notifications
You must be signed in to change notification settings - Fork 104
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
feat: Added initial Trimming support. #21
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! The core fixes look good! I just have a question and a change request I commented inline in the sources.
@@ -6,6 +6,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenAI", "src\OpenAI.csproj | |||
EndProject | |||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenAI.Examples", "examples\OpenAI.Examples.csproj", "{1F1CD1D4-9932-4B73-99D8-C252A67D4B46}" | |||
EndProject | |||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "TrimmingHelper", "helpers\TrimmingHelper\TrimmingHelper.csproj", "{4C6C7FB5-DD4E-44A8-9CBF-D739284FDC23}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want the TrimmingHelper project in this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will delete this after the review, right now it is still needed for re-checking
@@ -7,7 +7,7 @@ | |||
<VersionPrefix>2.0.0</VersionPrefix> | |||
<VersionSuffix>beta.3</VersionSuffix> | |||
|
|||
<TargetFrameworks>netstandard2.0;net6.0</TargetFrameworks> | |||
<TargetFrameworks>netstandard2.0;net6.0;net8.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for adding this explicit target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that attributes only appeared in net7, I'm not sure that net6 issues all the appropriate warnings for trimming, so I always try to add net8 to see actual problems.
Also, adding net8 allows you to avoid using polyfills for it and opens up access to other optimizations, as well as up-to-date diagnostics / in some cases more complete nullability markup
I can remove this, it can be added later if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub, what are your thoughts on this? We have been trying to minimize the number of cross-targets, as the matrix can become supper messy after a while, but maybe adding net 8 is not too bad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want net8.0 at least by Nov when net6.0 goes out of support. Might as well add it now, and gain the additional benefits.
Closes #20
But because preview version of System.Memory.Data does not explicitly support net6.0, this warning appears hereThere are two ways out:Ignore, this can work without problemsAdd #ifdef/ MSBuild Conditions to use old behavior for net6 and new for net8/netstandardI'm waiting for feedback