Skip to content

[Cogs Face] Overall Refinement (renaming, refactoring, bug fixing, reference refining)#2192

Merged
fearthecowboy merged 12 commits intoAzure:masterfrom
lebronJ:OverallRefactor
Jan 3, 2018
Merged

[Cogs Face] Overall Refinement (renaming, refactoring, bug fixing, reference refining)#2192
fearthecowboy merged 12 commits intoAzure:masterfrom
lebronJ:OverallRefactor

Conversation

@lebronJ
Copy link
Copy Markdown
Contributor

@lebronJ lebronJ commented Dec 21, 2017

Hi,

I am a member of Face SDK team, focusing on Cogs Face API. This PR is mainly to improve the Face API Swagger file by (See comment below for details)

  1. Renaming for better understanding
  2. Refactoring object types (by extracting shared parameters/definitions and using inheritance)
  3. Fixing existing bugs
  4. Refining SDK references

This PR is based on @DavidLiCIG 's former version and has been reviewed internally with teammates @huxuan by generating .NET SDK to check the outcome SDK structure and validate all unit tests.

@huxuan
Copy link
Copy Markdown

huxuan commented Dec 21, 2017

Confirmed for the internal review. And review process can be referred here: lebronJ/azure-sdk-for-net#1

@lebronJ
Copy link
Copy Markdown
Contributor Author

lebronJ commented Dec 21, 2017

Detailed major changes in this PR:

  1. Extract shared faceUserData, faceListId, personGroupId, personId and persistedFaceId as request parameter
  2. Extract NameAndUserDataContract as
    a) request body for creating/updating person/personGroup/faceList and
    b) base class of person/personGroup/faceList
  3. Fix wrong nullable types
  4. Correct/Refine comments
  5. Rename and refactor object types for better understanding:
    a) Person, PersonGroup, PersonGroupPerson, FaceList, PersistedFace, DetectedFace
    b) VerifyFaceToFaceRequest and VerifyFaceToPersonRequest
    c) Remove trailing Properties for face attributes (eg. FacialHairProperties to FacialHair)
  6. Extract shared Confidence and Level for number threshold of [0-1]
  7. Fix incorrect examples

@azuresdkciprbot
Copy link
Copy Markdown

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/cognitiveservices/data-plane/Face/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@marstr marstr changed the base branch from current to master December 28, 2017 17:51
Copy link
Copy Markdown
Contributor

@DavidLiCIG DavidLiCIG left a comment

Choose a reason for hiding this comment

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

Looks good!

@azuresdkciprbot
Copy link
Copy Markdown

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/cognitiveservices/data-plane/Face/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@lebronJ
Copy link
Copy Markdown
Contributor Author

lebronJ commented Jan 2, 2018

Force push to rebase onto the latest master branch

@fearthecowboy
Copy link
Copy Markdown
Contributor

A couple content-type mismatches in examples: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/324014381#L679

@huxuan
Copy link
Copy Markdown

huxuan commented Jan 3, 2018

Hi @fearthecowboy, the content type error should be a known issue as explained here #2166 (comment).

The content type errors have to do with Azure/oav#136 (Azure/oav#167)

@fearthecowboy
Copy link
Copy Markdown
Contributor

@huxuan Ah, right thanks! (FYI, in follow up PRs, we should probably note that in the PR comment ahead so that the next reviewer (aka 'future @fearthecowboy') knows.

😁

@fearthecowboy
Copy link
Copy Markdown
Contributor

If y'all are happy, I'm happy, so I can merge anytime. OK to Merge?

@DavidLiCIG
Copy link
Copy Markdown
Contributor

DavidLiCIG commented Jan 3, 2018 via email

@fearthecowboy fearthecowboy merged commit db22f73 into Azure:master Jan 3, 2018
@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-python

@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-python

@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-ruby

@huxuan
Copy link
Copy Markdown

huxuan commented Jan 4, 2018

@fearthecowboy Gotcha, will do it in the following pull requests.
@lebronJ just for reminder~Good Job, man.

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