Skip to content

Conversation

@AArnott
Copy link
Contributor

@AArnott AArnott commented Aug 24, 2021

The Switch base class that this value is passed to already is marked as allowing a null value here.

@ghost
Copy link

ghost commented Aug 24, 2021

Tagging subscribers to this area: @tommcdon, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

The Switch base class that this value is passed to already is marked as allowing a null value here.

Author: AArnott
Assignees: -
Labels:

area-System.Diagnostics

Milestone: -

@AArnott
Copy link
Contributor Author

AArnott commented Aug 24, 2021

Is this the right target branch to target net6.0?

@stephentoub
Copy link
Member

stephentoub commented Aug 24, 2021

A few things:

  1. Changes need to first be made into main. If approved for backporting to 6 the change can be ported to release/6.0.
  2. For this change to be meaningful, it would also need to update the ref, not just the src.
    https://github.com/dotnet/runtime/blob/a7463b2a737dd9d0717e816a1644c96ce33238e5/src/libraries/System.Diagnostics.TraceSource/ref/System.Diagnostics.TraceSource.cs#L64
  3. Some of the related annotation doesn't look right at quick glance, stemming from this:

@AArnott AArnott changed the base branch from release/6.0-rc1 to main August 24, 2021 18:07
The `Switch` base class that this value is passed to already is marked as allowing a null value here.
@AArnott
Copy link
Contributor Author

AArnott commented Aug 24, 2021

Thanks, @stephentoub. I've updated the ref source as well, and retargeted to main for now.
To your point about there being evidence that maybe null shouldn't be passed in, or at least that other annotations are wrong, I see your point. @tommcdon is looking for an owner for this area that may be able to pursue this to make any additional corrections.

@stephentoub
Copy link
Member

stephentoub commented Aug 25, 2021

Just looking at this more deeply, I'm not sure this is a valid change. For example, this fails with an argument null exception:

var s = new SourceSwitch("myswitch", null);
var a = s.Attributes;

I think this means this PR shouldn't be merged, and someone needs to look at the base annotations again.
cc: @jeffhandley, @buyaa-n

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 25, 2021

  1. Some of the related annotation doesn't look right at quick glance, stemming from this:

Right looks _defaultValue shouldn't be nullable

The Switch base class that this value is passed to already is marked as allowing a null value here.

@AArnott the base class annotation is wrong, sorry about that, i am gonna close this PR and will update the base annotation

@buyaa-n buyaa-n closed this Aug 25, 2021
@AArnott AArnott deleted the AArnott-patch-1 branch August 25, 2021 13:45
@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2021
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