Target .NET Standard 2.0 #89
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #89 +/- ##
==========================================
- Coverage 93.36% 93.29% -0.08%
==========================================
Files 77 77
Lines 4568 4576 +8
Branches 422 423 +1
==========================================
+ Hits 4265 4269 +4
- Misses 181 185 +4
Partials 122 122
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| @@ -82,8 +82,8 @@ public async Task<long> DelAsync(RedisKey key, string? path = null) | |||
| var res = await _db.ExecuteAsync(JsonCommandBuilder.Get<T>(key, path)); | |||
| if (res.Type == ResultType.BulkString) | |||
| { | |||
| var arr = JsonSerializer.Deserialize<JsonArray>(res.ToString()!); | |||
| if (arr?.Count > 0) | |||
| var arr = JsonSerializer.Deserialize<JsonElement[]>(res.ToString()!); | |||
There was a problem hiding this comment.
Is there a reason to move this to a JsonElement[] as opposed to a JsonArray?
There was a problem hiding this comment.
After I added netstandard2.0 to TargetFrameworks, I got this error:
The type or namespace name 'JsonArray' could not be found (are you missing a using directive or an assembly reference?) [NRedisStack]csharp(CS0246)
There was a problem hiding this comment.
It's because you are using an older version of System.Text.Json - I think you SHOULD be able to switch it to 7.0.2 (current version) without breaking compatibility. The best way to validate that would be to make sure that
points to some older versions of .NET Framework (e.g. 4.5.2)There was a problem hiding this comment.
do you mean to change the Version in this line: <PackageReference Include="System.Text.Json" Version="4.7.1" Condition="'$(TargetFramework)' == 'netstandard2.0'" />
to 7.0.2?
There was a problem hiding this comment.
No - so in this case we want to also test against an additional framework (just to make sure that the library is going to be fully compatible with that framework) .NET 4.5.2 is the oldest supported framework in .NET Standard 2.0 - so you just add net452 to the semi-colon delaminated list of target frameworks. You will need to enable this to run on the windows-latest runner in your integration job since .NET Framework can't run on Ubuntu. And you'll need to adjust your dotnet test command to specify the frameworks with the -f option in each job. See docs here
src/NRedisStack/Json/JsonCommands.cs
Outdated
| @@ -207,8 +207,8 @@ public RedisResult Get(RedisKey key, string[] paths, RedisValue? indent = null, | |||
| var res = _db.Execute(JsonCommandBuilder.Get<T>(key, path)); | |||
| if (res.Type == ResultType.BulkString) | |||
| { | |||
| var arr = JsonSerializer.Deserialize<JsonArray>(res.ToString()!); | |||
| if (arr?.Count > 0) | |||
| var arr = JsonSerializer.Deserialize<JsonElement[]>(res.ToString()!); | |||
| @@ -82,8 +82,8 @@ public async Task<long> DelAsync(RedisKey key, string? path = null) | |||
| var res = await _db.ExecuteAsync(JsonCommandBuilder.Get<T>(key, path)); | |||
| if (res.Type == ResultType.BulkString) | |||
| { | |||
| var arr = JsonSerializer.Deserialize<JsonArray>(res.ToString()!); | |||
| if (arr?.Count > 0) | |||
| var arr = JsonSerializer.Deserialize<JsonElement[]>(res.ToString()!); | |||
There was a problem hiding this comment.
It's because you are using an older version of System.Text.Json - I think you SHOULD be able to switch it to 7.0.2 (current version) without breaking compatibility. The best way to validate that would be to make sure that
points to some older versions of .NET Framework (e.g. 4.5.2)
.github/workflows/integration.yml
Outdated
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install curl -y && sudo apt-get install gpg -y && apt-get install lsb-release -y && apt-get install libgomp1 -y | ||
| curl https://packages.redis.io/redis-stack/redis-stack-server-6.2.6-v6.jammy.x86_64.tar.gz -o redis-stack.tar.gz |
There was a problem hiding this comment.
nit: ${{env.redis_stack_version}}
.github/workflows/integration.yml
Outdated
| - name: Test | ||
| shell: cmd | ||
| run: | | ||
| START wsl ./redis-stack-server-6.2.6-v6/bin/redis-stack-server & |
There was a problem hiding this comment.
Same ${{env.redis_stack_version}}
| @@ -1,9 +1,10 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
| <TargetFrameworks>net6.0;net7.0</TargetFrameworks> | |||
| <TargetFrameworks>net6.0;net7.0;net481</TargetFrameworks> | |||
There was a problem hiding this comment.
Question for my own knowledge 481 because it's the latest in 4?
There was a problem hiding this comment.
yes - .NET Framework 4.8.1 is the latest (and likely final) framework version. It's also one of two framework versions that ship natively on the windows-latest image
No description provided.