Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Text Analytics] Adding analyze and healthcare APIs #11375
[Text Analytics] Adding analyze and healthcare APIs #11375
Changes from 1 commit
cdec7a2
cfafeb2
2c2e35f
4a12db1
61e8b83
8e573f3
8fdecef
2a5f012
85bf1a0
b6fbcf7
6315104
a8dc5e0
2abcf62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't really understand
BeginAnalyzeHealthcareOptions
andBeginAnalyzeOptions
having custom subkeys that are just copies ofTextAnalyticsOperationOptions
with no customization.I would expect this to instead look like
or possibly
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.
I think you are discussing two things here:
analyze
key is typed asAnalyzeJobOptions
which is just a synonym toTextAnalyticsOperationOptions
, so why not type it asTextAnalyticsOperationOptions
instead? I think having a distinct relevant name makes a better user experience with intellisense and it makes it easier to extend this type in the future.BeginAnalyzeOptions
? I think keeping the options type as is makes it clear to the user which options are used to configure the analyze job itself and which are used to configure the poller.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 this for open-ended extensibility? Should we instead make this
string
and haveKnownWarningCodes
or something?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.
Hey @joheredi, is this related to a recent change in the code generator? This is the swagger definition: https://github.com/Azure/azure-rest-api-specs/blob/28d37f8847253787a56d39814e7ccd5194aeeabe/specification/cognitiveservices/data-plane/TextAnalytics/preview/v3.1-preview.3/TextAnalytics.json#L815
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.
Yeah this is the way the generator is doing "extensible" enums. I don't think we have reach a formal agreement on how to generate these.
@xirzec your proposal is to instead of generating this:
Do this?
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.
The issue with this way of modeling extensible enums is that it is exactly the same as
type WarningCode = string;
. The information about known values is only contained in the source code text and is fundamentally erased from the type when it comes down to set theory. Even if we added aKnownWarningCodes
enum or type, the problem then is how to associate that type with the parameter in a discoverable way. It's not going to show up when you invoke completion in an IDE, for example.