#230 add in TFM's so dependencies can be optimised #231
Conversation
|
I added some comments per-file, but other than that I get the spirit of your PR and I agree: less explicit dependencies for TFMs that do not require them, great! In case you agree about the changes, let me know if you want to make them yourself or I can do them. Thanks! |
|
Happy to make changes @jodydonetti but atm I can't see any proposed changes. Is there a chance you are yet to click finish on your review which publishes the comments? |
There was a problem hiding this comment.
Here the TFM should be net7 and not net6, I think, since MemoryPack targets explicitely net7 for some optimizations, see here.
Makes sense?
There was a problem hiding this comment.
Why targeting net3.1 explicitly since it's not supported anymore?
Honest question, I've never multi targeted like this before so I'm trying to better understand the TFMs selection strategy here.
There was a problem hiding this comment.
Same as the other comment, selecting net core 3.1 enables that framework and higher to benefit of not adding the dependency. Net 6 was for consistent but we could remove it if desired.
There was a problem hiding this comment.
I'm looking right now at this as an inspiration and to double check.
There too I can see net6.0 in the <TargetFrameworks> element, even though below there are no conditions targeting that... I'm trying to wrap my head around this.
There was a problem hiding this comment.
Same thing as above: why targeting net3.1 explicitly since it's not supported anymore?
There was a problem hiding this comment.
adding net core 3.1 enables net 5 projects to also not have the dependency. Net 6 is to keep it consistent with the other location but could remove it as well,
There was a problem hiding this comment.
Same as above about net3.1.
There was a problem hiding this comment.
The reason i did the 3 frameworks was that System.Threading.Tasks.Extensions was added in netcoreapp 3.1 however System.Diagnostics.DiagnosticSource was only added in net 6.
There was a problem hiding this comment.
Got it, but if System.Diagnostics.DiagnosticSource was only added in net 6, why its condition is "'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'netcoreapp3.1'"?
I would've expected it has "'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'net6.0'", or am I missing something?
Again, be patient, first time dealing with these kinds of issues 😅
There was a problem hiding this comment.
No problem, the reason for the condition being like this is so that it only adds the dependency when it wasn't a part of the framework. If we were to add net6.0 then it would be included.
Yep, I forgot to submit the review, sorry about that 🤦 |
|
No problem. Review Comments have just been published so will look at them shortly & come back/make changes. |
…thub.com/thompson-tomo/FusionCache into chore/#230_AddTFMToOptimisedDependencies
…location in happy path)
Add in net coreapp 3.1 & net 6 as TFM to a number of projects so that conditions can be placed on the dependencies to removed explicit dependencies on the newer frameworks
Closes #230