-
Notifications
You must be signed in to change notification settings - Fork 772
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
Extract TFM for DLLs shipped from this repo #4903
Conversation
@@ -3,7 +3,12 @@ | |||
|
|||
<PropertyGroup> | |||
<!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. --> | |||
<DefaultTargetFrameworks>net6.0;netstandard2.0;net462</DefaultTargetFrameworks> | |||
<TargetFrameworksForLibraries>net6.0;netstandard2.0;net462</TargetFrameworksForLibraries> | |||
<TargetFrameworksForLibrariesExtended>net6.0;netstandard2.1;netstandard2.0;net462</TargetFrameworksForLibrariesExtended> |
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.
curious - what's the rationale behind this naming? (I saw it's used for OTLP exporter and OpenTelemetry project.)
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.
Just a name for libs that need to target extra netstandard TFM.
My hope is that after this PR:
- All the TFMs are centrally managed in this single file. It'll help us to manage / review TFMs in the future (e.g. when we add net8 or retire net7), and stop bleeding as new libs being added.
- We will do the same for tests/examples.
- We will keep reducing the number of different targets, ideally I hope that we end up with 3 combinations: TargetFrameworksForLibraries, TargetFrameworksForTests, TargetFrameworksForExamples
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.
Possible to do this here...
<TargetFrameworksForLibrariesExtended>$(TargetFrameworksForLibraries);netstandard2.1</TargetFrameworksForLibrariesExtended>
...or order needs to be preserved?
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 feel inheritance is not adding much value, and is creating issue for IDEs/tools.
Ultimately we should try to align all the TFMs by reducing the number of variations IMHO.
Codecov Report
@@ Coverage Diff @@
## main #4903 +/- ##
==========================================
+ Coverage 82.94% 83.16% +0.22%
==========================================
Files 294 294
Lines 12200 12200
==========================================
+ Hits 10119 10146 +27
+ Misses 2081 2054 -27
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -3,7 +3,12 @@ | |||
|
|||
<PropertyGroup> | |||
<!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. --> | |||
<DefaultTargetFrameworks>net6.0;netstandard2.0;net462</DefaultTargetFrameworks> | |||
<TargetFrameworksForLibraries>net6.0;netstandard2.0;net462</TargetFrameworksForLibraries> |
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.
since everything in prod.props is a Library anyway, we could use diff. name by avoiding "library"...
DefaultTargetFrameworks, DefaultTargetFrameworksExtended seems okay to me plus the
TargetFrameworksForAspNetCoreInstrumentation, etc..
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 feel the word "default" is generating confusion. The way I look at it is that:
- There is no "default", all of them are explcit.
- Having "TargetFrameworksForXyz" makes it more consistent.
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.
If you look at what I was doing in #4902, I consider this as "default" (the individual projects don't have to add TargetFrameworks at all since the props file covered it).
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 agree with @cijothomas I don't love "Libraries" in the name because all of these things are really libraries in some regard but I'm fine to leave as-is and we can continue to mess with it 😄
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.
Agree with making TFM into central place.
No strong opinion on what names to use, its immaterial for end users anyway.
No description provided.