Skip to content

Conversation

@azabbasi
Copy link
Contributor

@azabbasi azabbasi commented Oct 27, 2020

This change will render certain options look empty to users so we are removing them from the API signatures.

We have the option to just mark some of the options internal and do not implement any overrides in customized classes, but that will result in TraceParent and TraceState have the incorrect naming format : Tracestate, Traceparent

Copy link
Contributor

@drwill-ms drwill-ms left a comment

Choose a reason for hiding this comment

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

I really don't like the regions for internal properties, but other than that, looks good.

@tg-msft
Copy link
Member

tg-msft commented Oct 27, 2020

If you're making the Options classes internal, do they get set as headers by the generated code? Does that conflict with https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/src/Pipeline/Internal/RequestActivityPolicy.cs? // cc @pakrym as the expert here

@tg-msft
Copy link
Member

tg-msft commented Oct 27, 2020

Yeah, that was my advice because the RequestActivityPolicy sets them automatically. However it won't replace them if already set. I just want to double check the generated code doesn't cause a problem. If it does, we could also just yank all the traceparent/tracestate headers from your swagger with a README transform in our repo.

@timtay-microsoft
Copy link
Member

it won't replace them if already set.

We shouldn't be passing down any values for these to the generated code, so we should be fine here

@pakrym
Copy link
Contributor

pakrym commented Oct 27, 2020

FYI #16318 (comment) coming

@azabbasi azabbasi merged commit b3f9b17 into master Oct 27, 2020
@azabbasi azabbasi deleted the feature/adt/azabbasi/optionsUpdate branch October 27, 2020 21:29
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants