-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Vision OCR multi-region #911
Conversation
Hi @telpirion, some of the vision tests are failing on this PR. Let me know when you hace check those and I can review. |
vision/api/Detect/Program.cs
Outdated
private static object DetectTextWithLocation(Image image) | ||
{ | ||
// [START vision_set_endpoint] | ||
|
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.
Extraneous newline. I looked at the other samples and this isn't present in the other samples.
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.
Done.
vision/api/Detect/Program.cs
Outdated
Endpoint = new ServiceEndpoint("eu-vision.googleapis.com") | ||
}.Build(); | ||
// [END vision_set_endpoint] | ||
|
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.
Extraneous newline. Besides these 2 extra lines (I looked at the other samples in this file and they don't have empty newlines), all LGTM
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.
Done.
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.
Thank you!
Hi @telpirion, I fixed the lint issue on Storage but the Vision tests are still failing on this PR (apparently there's a file missing somewhere).
|
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.
LGTM with a very minor nit.
vision/api/Detect/Program.cs
Outdated
{ | ||
// [START vision_set_endpoint] | ||
// Instantiate a client connected to the 'eu' location. | ||
var client = new ImageAnnotatorClientBuilder() |
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.
nit: Remove the (), they are not needed on object initializers.
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.
Did not know that! Thank you.
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.
LGTM! Happy for you to merge.
This pull request adds the following region_tag:
vision_set_endpoint
The canonical sample is here:
GoogleCloudPlatform/python-docs-samples#2569