SmartFormat
SmartFormat copied to clipboard
#380 Add net 6 to System.Text.Json extension with conditional dependencies
Makes eliminated packages where they can be provided by the framework by using conditions and added net 6 to STJ so that it can also become optional.
Closes #380
@thompson-tomo Thanks! Please have a look at the failing build on appveyor for this PR.
@axunonb have gone and added an additional dependency to ZString to resolve the build issue.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 96%. Comparing base (
b438ccd
) to head (2b45a8a
).
Additional details and impacted files
@@ Coverage Diff @@
## version/3.5.0 #381 +/- ##
============================================
Coverage 96% 96%
============================================
Files 92 92
Lines 3233 3233
============================================
Hits 3117 3117
Misses 116 116
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for the fix. If we added net60, shouldn't this cover the main and all extension projects? We'll have a closer look during next days.
Hmm, there is 2 trains of thought:
- Keep framework's consistent across libraries
- only target needed frameworks ie based on needs (dependency reduction, AOT, trimming etc)
I don't have a strong opinion either way but do tend for send one due to package size reduction.
Adding NET60 as a target framework to only 1-2 projects is not enough. We're ready to go this step, but for all SmartFormat projects. Cysharp/ZString released v2.6.0 lately, which also targets NET60 and NET70. All this requires a deeper look into all consequences and possible issues. Maybe we can finally reference Cysharp/ZString as a package directly. So a first step to go might be to migrate Cysharp/ZString to v2.6.0, and afterwards add NET60 to all projects - including the conditional dependencies you're suggesting.
Have gone and cleaned up properties which has allowed me to bump all packages in 1 hit. I have overridden the TFM for the base package due to nullability issues which someone should look at.
We've created a new branch v3.5.0.
It already includes necessary changes to reference the Cysharp ZString
Nuget package.
To continue with this PR please rebase to this new branch. It would be great to cover all (extension) projects the way you propose for System.Text.Json
.
After merging the PR, a final step is to update conditional compilation in the current code base, and to update package references to the latest versions. We'll start with this workload now.
@axunonb requested changes have been made and all builds now.
LGTM, thanks for the PR and your initiative.