Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move DiagnosticsHandler.IsGloballyEnabled back to a method #55351

Merged
merged 2 commits into from
Jul 9, 2021
Merged

Move DiagnosticsHandler.IsGloballyEnabled back to a method #55351

merged 2 commits into from
Jul 9, 2021

Conversation

steveisok
Copy link
Member

@steveisok steveisok commented Jul 8, 2021 •
edited
Loading

Intent is to make linker substitutions happy and try to unblock dotnet/sdk#18801

Move DiagnosticsHandler.IsGloballyEnabled back to a method
4205ae8 
Intent is to make linker substitutions happy
@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Intent is to make linker substitutions happy

Author: steveisok
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@steveisok steveisok requested review from antonfirsov and karelz July 8, 2021 17:20
@steveisok steveisok mentioned this pull request Jul 8, 2021
Merged
@steveisok
Copy link
Member Author

@vitek-karas @eerhardt Given the current code structure, is the linker substitution going to work?

jeffschwMSFT
jeffschwMSFT approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks @steveisok

@jeffschwMSFT jeffschwMSFT requested a review from ericstj July 8, 2021 17:25
@captainsafia captainsafia mentioned this pull request Jul 8, 2021
Merged
vitek-karas
vitek-karas approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

You could change it to a property, you just need to update the ILLink.Substitutions.xml for that assembly to now point to the property getter - that should work as well.

ManickaP
ManickaP approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM
cc @MihaZupan

@steveisok @ManickaP
Update src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsH…
d1b6727 
…andler.cs

Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@steveisok steveisok merged commit 03cb946 into dotnet:main Jul 9, 2021
@steveisok steveisok deleted the fix-handler-linker-sub branch July 9, 2021 03:17
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eerhardt eerhardt eerhardt approved these changes

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

@vitek-karas vitek-karas vitek-karas approved these changes

@ManickaP ManickaP ManickaP approved these changes

@MihaZupan MihaZupan MihaZupan approved these changes

@antonfirsov antonfirsov Awaiting requested review from antonfirsov

@karelz karelz Awaiting requested review from karelz

@ericstj ericstj Awaiting requested review from ericstj

Assignees
No one assigned
Labels
area-System.Net.Http
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@steveisok @eerhardt @jeffschwMSFT @vitek-karas @ManickaP @MihaZupan @karelz