Conversation
|
Hi, @shmed Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vsswagger@microsoft.com |
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
Swagger Generation Artifacts
|
|
Hi @shmed, Your PR has some issues. Please fix the CI sequentially by following the order of
|
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchindex.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
| } | ||
| }, | ||
| { | ||
| "name": "semanticConfiguration", |
There was a problem hiding this comment.
In Python this will end up getting exposed as semantic_configuration_name. Is that the case for other languages? If so, perhaps an x-ms-client-name would be appropriate.
There was a problem hiding this comment.
I'm not familiar with how names are generated in various languages, though I do want to make sure it's consistent across parameters. For this specific parameter, I was following the footstep of the "scoringProfile" parameter above. If the scoringProfile parameter gets translated into scoring_profile_name in python, then I would want the same for semanticConfiguration.
There was a problem hiding this comment.
They don't get translated that way--in our convenience layer we will have to manually rename it. If all languages would expose this as SemanticConfigurationName, then it would be useful to have the x-ms-client-name here. @Mohit-Chakraborty @alzimmermsft @sarangan12
There was a problem hiding this comment.
I personally dont have preferences as to whether we should call it SemanticConfigurationName or SemanticConfiguration, all I want to be sure of is that we follow the pattern of the other parameter defined right above this one (ScoringProfile). Right now, ScoringProfile does not have an x-ms-client-name property, so I want to make sure we dont end up in a situation where ScoringProfile and SemanticConfiguration end up with different naming patterns.
There was a problem hiding this comment.
At least for Java, this will result in a private property called semanticConfiguration and getter and setter methods getSemanticConfiguration and setSemanticConfiguration. For the most part Java code generation leaves the Swagger name field untouched and uses it directly as-is.
| }, | ||
| "description": "Ranking function based on the Okapi BM25 similarity algorithm. BM25 is a TF-IDF-like algorithm that includes length normalization (controlled by the 'b' parameter) as well as term frequency saturation (controlled by the 'k1' parameter)." | ||
| }, | ||
| "Semantic": { |
There was a problem hiding this comment.
Should this be SemanticConfigurations?
There was a problem hiding this comment.
semanticConfigurations is the name of one of the nested property under "semantic" will contain more property in the future (including defaultSemanticConfiguration and maybe a few others)
There was a problem hiding this comment.
There is no issue with a property having the same name as a class, AFAIK.
SemanticConfigurations.semantic_configurations is perfectly valid, if somewhat redundant. We don't want classes generated called Semantic.
|
NewApiVersionRequired reason: |
| }, | ||
| "Semantic": { | ||
| "properties": { | ||
| "semanticConfigurations": { |
There was a problem hiding this comment.
Based on my above comment, I would recommend calling this configurations. Then you would have SemanticConfigurations.configurations which is less rendundant.
| "url": "https://docs.microsoft.com/azure/search/index-ranking-similarity" | ||
| } | ||
| }, | ||
| "semantic": { |
There was a problem hiding this comment.
This is not a good property name. It should probably be called semanticConfigurations.
tjprescott
left a comment
There was a problem hiding this comment.
I didn't include my comments in a review (whoops) but they are in there. Mostly naming issues.
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
|
@shmed please reopen this and target the Azure:main branch. Ensure you tag me as the assignee and reference this PR. |
Created this new PR -> #16642 I can't seem to find how to change the "assignee" though. Can you help? @tjprescott |
Early draft describing the new semantic configuration
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Add a changelog entry for this PR by answering the following questions:
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Otherwise your PR may be subject to ARM review requirements. Complete the following:
Check this box if any of the following apply to the PR so that label “WaitForARMFeedback” will be added automatically to begin ARM API Review. Failure to comply may result in delays to the manifest.
-[ ] To review changes efficiently, ensure you copy the existing version into the new directory structure for first commit and then push new changes, including version updates, in separate commits.
Ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If any of the following scenarios apply to the PR, request approval from the Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.