Skip to content

Conversation

@kg
Copy link
Member

@kg kg commented Mar 5, 2024

Draft PR because I'm not convinced this is the right fix.

@ghost ghost added the area-System.Text.Json label Mar 5, 2024
@ghost ghost assigned kg Mar 5, 2024
@ghost
Copy link

ghost commented Mar 5, 2024

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: kg
Assignees: kg
Labels:

area-System.Text.Json

Milestone: -

if (json.HasValueSequence)
{
Assert.True(json.ValueSpan == default);
Assert.True(json.ValueSpan.IsEmpty);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests wants to really test for default. This build break should be fixed by pragma warning disable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's my thinking too. I wanted to see if there are more breaks after these ones are fixed.

Copy link
Contributor

@buyaa-n buyaa-n Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the analyzer should be disabled for test projects, could you please add below rows here:

# CA2261: Do not use ConfigureAwaitOptions.SuppressThrowing with Task<TResult>
dotnet_diagnostic.CA2261.severity = none

# CA2265: Do not compare Span<T> to 'null' or 'default'
dotnet_diagnostic.CA2265.severity = none

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the analyzer updated reverted, this can be added later when analyzer updated

@jkotas jkotas requested a review from buyaa-n March 5, 2024 20:16
@vcsjones
Copy link
Member

vcsjones commented Mar 5, 2024

There are still more build issues this PR does not address, like in src/libraries/System.Runtime/tests/System.Runtime.Tests/:

/Users/vcsjones/Projects/runtime/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs(1085,25): error CA2265: Comparing a span to 'default' might not do what you intended, make the code more explicit by checking 'IsEmpty' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2265) [/Users/vcsjones/Projects/runtime/src/libraries/System.Runtime/tests/System.Runtime.Tests/System.Runtime.Tests.csproj::TargetFramework=net9.0-unix]

@kg kg closed this Mar 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants