SmartFormat icon indicating copy to clipboard operation
SmartFormat copied to clipboard

#380 Add net 6 to System.Text.Json extension with conditional dependencies

Open thompson-tomo opened this issue 10 months ago • 7 comments

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 avatar Apr 17 '24 09:04 thompson-tomo

@thompson-tomo Thanks! Please have a look at the failing build on appveyor for this PR.

axunonb avatar Apr 17 '24 11:04 axunonb

@axunonb have gone and added an additional dependency to ZString to resolve the build issue.

thompson-tomo avatar Apr 17 '24 22:04 thompson-tomo

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.

codecov[bot] avatar Apr 17 '24 22:04 codecov[bot]

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.

axunonb avatar Apr 19 '24 10:04 axunonb

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.

thompson-tomo avatar Apr 19 '24 10:04 thompson-tomo

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.

axunonb avatar Apr 28 '24 19:04 axunonb

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.

thompson-tomo avatar Apr 29 '24 05:04 thompson-tomo

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 avatar May 10 '24 12:05 axunonb

@axunonb requested changes have been made and all builds now.

thompson-tomo avatar May 12 '24 00:05 thompson-tomo

LGTM, thanks for the PR and your initiative.

axunonb avatar May 12 '24 11:05 axunonb