Skip to content

renaming custom search to custom web search#4332

Closed
q3blend wants to merge 2 commits intoAzure:psSdkJson6from
q3blend:customsearch
Closed

renaming custom search to custom web search#4332
q3blend wants to merge 2 commits intoAzure:psSdkJson6from
q3blend:customsearch

Conversation

@q3blend
Copy link
Contributor

@q3blend q3blend commented May 17, 2018

Description


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@dsgouda
Copy link
Contributor

dsgouda commented May 17, 2018

@q3blend Please fix the test failures

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Please update the code generation path with cognitiveservices/data-plane/CustomWebSearch in the generate.ps1
Also, what is the reason behind making these changes?

Updating output path of custom web search generation
@dsgouda
Copy link
Contributor

dsgouda commented May 22, 2018

@q3blend would like to know the reason behind these changes. Do you intend to modify the package name too moving forward?
We usually recommend directory names to match the service/RP
@shahabhijeet PTAL

@dsgouda dsgouda requested a review from shahabhijeet May 22, 2018 16:23
@q3blend
Copy link
Contributor Author

q3blend commented May 23, 2018

@dsgouda Initially we only had 1 version of customsearch, web. Now we have web and image, so it's better to name them customwebsearch and customimagesearch. I am not intending to change the package name at this stage as it would be an unnecessary breaking change

@dsgouda
Copy link
Contributor

dsgouda commented May 24, 2018

@shahabhijeet can you confirm if this directory structure is acceptable.

@shahabhijeet
Copy link
Contributor

shahabhijeet commented May 24, 2018

@q3blend does that mean your nuget package story will be as below:

  1. Microsoft.Azure.CognitiveServices.Search.CustomSearch
  2. Microsoft.Azure.CognitiveServices.Search.CustomImageSearch
    If yes, then what are you actually trying to solve, just make project code directory structure much more intuitive?

What is the reason for not releasing a new package with the right name?
Is documentation going to still refer to your service/package as CustomSearch or CustomWebSearch?

@q3blend
Copy link
Contributor Author

q3blend commented May 27, 2018

@shahabhijeet yes, the reasoning is making the directory structure more intuitive. Releasing a new package would be an unnecessary braking change at this point, and a lot of extra documentation work. We still refer to it as CustomSearch in the documentation. There are currently no plans on updating that. We have also updated the spec directory to follow the same structure (already merged)

@shahabhijeet
Copy link
Contributor

@q3blend if you think changing nuget package is unnecessary, so is changing directory name without changing project/solution/test project names.
Please bear in mind, there are roughly 70+ services and changing directory name for 1 project without changing the actual product name will cause confusion. Plus being open source, this creates problems for our external contributors to know what directory hold which service project.
So I am sorry, unless you are trying to align the entire directory and project name as well as nuget name, we will not be able to accept this chagne in it's current state.

@q3blend
Copy link
Contributor Author

q3blend commented May 29, 2018

@shahabhijeet

  1. Changing the directory structure here is not a breaking change, unlike changing it in nuget
  2. This change is already merged for the spec repo. This will make the 2 repos different, which is even more confusing for the open source community
  3. Custom Search and Bing Search are not following the same naming conventions, which again is confusing for the community as most custom search customers are also familiar with bing search

@shahabhijeet
Copy link
Contributor

@q3blend this is not about breaking change. So I am not sure how breaking change is part of this discussion.
We try to maintain service name/package name and directory name consistent.
If you have decided to change directory structure without changing the nuget package name, that is where I have an issue and would like to know why you would like to do this at this state.
Why not make that change when you actually change the nuget package change.
Also provide a link to your change to the spec change, this should have been added to this PR to make sure the person who is reviewing gets the full context.
And finally, directory structure in spec repo has absolutely no relation to the directory structure here.
Because not every package for every service is named the same way, it differs from language to language.
This PR cannot be accepted at this stage with the chagne in name because I still have not heard a single reason why this has to be chagned this now wihtout changing the nuget package name.

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.

3 participants