-
Notifications
You must be signed in to change notification settings - Fork 3k
computer vision quickstart #4255
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
Conversation
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
#hold-off Please don't begin to review this yet. It isn't done |
Codecov Report
@@ Coverage Diff @@
## master #4255 +/- ##
==========================================
- Coverage 53.38% 49.48% -3.91%
==========================================
Files 10464 6276 -4188
Lines 219586 176038 -43548
==========================================
- Hits 117229 87110 -30119
+ Misses 102357 88928 -13429
Continue to review full report at Codecov.
|
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.
This is looking really good! Seeing it this way really makes this SDK seem easy to use, and I'm actually gonna give it a whirl now that I've seen this. 😉
For the examples, I think you can add a leeeetle more description to them. Brevity is certainly good, but if there are any gotchas or tips or simply something that'd be Good To Know for those operations, don't feel so hand-tied that you can't add a little nugget here and there. Then again, if there isn't much more to add, don't do so just 'cause I said add something (since I don't know the SDK or the service!).
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.
This is awesome, Dina. Pretty much exactly what I'd both want and expect to see in a README.
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.
Suuuper close!
I didn't tested PyPI with Markdown yet, but looks to me this follows the guideline and should be fine. |
@@ -4,3 +4,4 @@ package_pprint_name = "Cognitive Services Computer Vision" | |||
package_doc_id = "cognitive-services" | |||
is_stable = false | |||
is_arm = false | |||
autoupdate = false |
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.
It's actually auto_update
(my mistake, sorry)
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.
@diberry Looks like we have a merge conflict and need another edit (per @lmazuel). I can issue a PR into yours, or take all these changes and open a new PR under my account, then shepherd it through the process without needing to bug you again. 😄 Happy do do either--what's your preference?
@lmazuel @mmacy If I fix the merge conflicts, can the doc be merged today or is there something else that needs to be changed? Can we merge what is there then change in another PR if there are alterations. The original request is a month and a half old -- so just trying to figure out how to finish this. |
Me too. I'd love it if @lmazuel or someone on his team could sign up to be on-deck for merging this thing once the conflicts are resolved and you tag them to notify them when it's ready. I think what's happening is that it gets buried and doesn't come up on anyone's radar when it's actually ready to go. Once we get confirmation from Laurent that (other than the conflicts) it's ready to go, you can resolve the conflicts, then fire off some flares to explicitly notify them that it's ready for merge. |
@lmazuel Hey Laurent, what's the best path to take with this PR? It's been ready for merge a couple of different times now, but it's not been merged, and then conflicts creep in and it sits. I'm concerned that if I or Dina fix the conflicts we'll be in the same situation, so would love to know how we can best proceed. |
CC: @lmazuel |
@mmacy @loarabia I believe there is a misunderstanding here: I don't own this PR and I'm not the right person to decide what to do with it :). It comes from data-plane discussion on Cognitive Services, and I have no say in the content of this file if it matches the guidelines of @johanste I would suggest to fix the conflicts, rebase with master. be sure it passes CI (@scbedd can help to be sure CI is actually checking this README against the CI) and merge it :). I could do a final review if you wish in the form, but I let you guys decide the content ;) |
Do we still need to get this in? If so, it looks like we need to make sure someone that can merge is on point to merge when ready. |
I'll take an action item to get the conflicts resolved and the PR merged. |
Please note that there's been a couple of releases since this PR was opened. |
I'll look at that tomorrow, as repenting our sins on this PR length.... |
I have resolved the merge conflicts, so @lmazuel it's just a review of the content here. |
I updated the Readme with all changes since this PR was open, and executed manually all the samples to be sure they were working correctly. MERGING! |
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.
Checked-out the branch and tried them all, good to go
No description provided.