-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Add method-specific request options to TextAnalyticsClient #20049
Conversation
* Update public APIs * Fix nuget error in DemoApp by not packing it (non-shipping)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add CHANGELOG entries.
sdk/textanalytics/Azure.AI.TextAnalytics/api/Azure.AI.TextAnalytics.netstandard2.0.cs
Show resolved
Hide resolved
@@ -3,6 +3,7 @@ | |||
<PropertyGroup> | |||
<OutputType>Exe</OutputType> | |||
<TargetFramework>net5.0</TargetFramework> | |||
<IsPackable>false</IsPackable> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @ellismg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that just to let him know it's possible, or was this actually supposed to be up on nuget.org? I'm surprised this hasn't failed in the past. Though, I see from git blame
@ellismg added it.
If a "demo" directory sibling to "src" and "tests" is an emerging standard, we can update the build targets to add <IsPackable>false</IsPackable>
by default - as well as a few other tweaks to treat them more like tests or even samples. But, I would add that with Key Vault I was trying to start a standard on a "samples" subdirectory sibling to those directories or even the service directory itself, e.g. sdk/keyvault/samples that contain some samples not specific to a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as FYI as he owns TA.Protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the change here, as the intention was not to create anything that should be published to NuGet.org. The "demo" pattern isn't something I think we want to push long term, it was mainly there because having a small console program that we could use to test out how the API felt was useful and it helped us as a way to prototype some stuff.
I'm happy to just go delete this now if it's causing additional problems or confusion. We are not really using TA to prototype the low-level client work now (we have some other services we've been focused on) so I'm happy to remove the entire .Protocol
folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't feel it worth keeping, I'm all for reducing points of confusion.
Fixes #18482