Skip to content
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

Upgrades vision to partial-GAPIC #434

Merged
merged 11 commits into from
Jul 31, 2017
Merged

Upgrades vision to partial-GAPIC #434

merged 11 commits into from
Jul 31, 2017

Conversation

gguuss
Copy link
Contributor

@gguuss gguuss commented Jul 27, 2017

No description provided.

vision/detect.js Outdated
@@ -62,18 +59,20 @@ function detectFacesGCS (bucketName, fileName) {
// The path to the file within the bucket, e.g. "path/to/image.png"
// const fileName = 'path/to/image.png';

// Performs face detection on the remote file
vision.detectFaces(storage.bucket(bucketName).file(fileName))
const gcsPath = `gs://` + bucketName + `/` + fileName;
Copy link
Member

Choose a reason for hiding this comment

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

change to

const gcsPath = `gs://${bucketName}/${fileName}`;

vision/detect.js Outdated
vision.detectLabels(storage.bucket(bucketName).file(fileName))
.then((results) => {
const labels = results[0];
const gcsPath = `gs://` + bucketName + `/` + fileName;
Copy link
Member

Choose a reason for hiding this comment

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

change to

const gcsPath = `gs://${bucketName}/${fileName}`;

vision/detect.js Outdated
vision.detectLandmarks(storage.bucket(bucketName).file(fileName))
.then((results) => {
const landmarks = results[0];
const gcsPath = `gs://` + bucketName + `/` + fileName;
Copy link
Member

Choose a reason for hiding this comment

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

change to

const gcsPath = `gs://${bucketName}/${fileName}`;

vision/detect.js Outdated
vision.detectText(storage.bucket(bucketName).file(fileName))
.then((results) => {
const detections = results[0];
const gcsPath = `gs://` + bucketName + `/` + fileName;
Copy link
Member

Choose a reason for hiding this comment

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

change to

const gcsPath = `gs://${bucketName}/${fileName}`;

vision/detect.js Outdated
vision.detectLogos(storage.bucket(bucketName).file(fileName))
.then((results) => {
const logos = results[0];
const gcsPath = `gs://` + bucketName + `/` + fileName;
Copy link
Member

Choose a reason for hiding this comment

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

change to

const gcsPath = `gs://${bucketName}/${fileName}`;

vision/detect.js Outdated
@@ -621,10 +605,12 @@ function detectFulltextGCS (bucketName, fileName) {
// The path to the file within the bucket, e.g. "path/to/image.png"
// const fileName = 'my-file.jpg';

const gcsPath = `gs://` + bucketName + `/` + fileName;
Copy link
Member

Choose a reason for hiding this comment

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

change to

const gcsPath = `gs://${bucketName}/${fileName}`;

callback(null, faces);
})
.catch((err) => {
console.error('ERROR:', err);
Copy link
Member

Choose a reason for hiding this comment

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

Please add callback(err); below console.error

@@ -64,12 +67,18 @@ function highlightFaces (inputFile, faces, outputFile, Canvas, callback) {
context.strokeStyle = 'rgba(0,255,0,0.8)';
context.lineWidth = '5';

faces.forEach(function (face) {
faces.forEach((face) => {
Copy link
Member

Choose a reason for hiding this comment

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

Don't use an arrow function here if you're not going to convert the whole file to arrow functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: I converted the whole file to arrow functions. 😛

});
}
};
const cmd = `node quickstart.js`;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how the changes to this file could work. vision/quickstart.js has a hard-coded projectId (it's YOUR_PROJECT_ID). This test will result in failure. The hard-coded projectId in the quickstarts is the reason the quickstarts' tests mock the client library and intercept the call to replace YOUR_PROJECT_ID with a real project ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works because the API allows 10ish API calls before requiring the caller be authenticated. I'm pretty confident we can remove the code passing the project ID when the client library is initialized but I had assumed there was a reason we were passing the project ID explicitly (e.g. some EAP customers may need to do this).

Copy link
Member

Choose a reason for hiding this comment

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

We pass it explicitly because that's how all of our "quickstart" samples were designed. For the quickstarts we chose to be explicit about what project is being used.

});
vision.batchAnnotateImages({requests: requests})
.then((results) => {
// console.log(JSON.stringify(results, undefined, 2));
Copy link
Member

Choose a reason for hiding this comment

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

Remove this line.

Copy link
Contributor Author

@gguuss gguuss left a comment

Choose a reason for hiding this comment

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

Looks like @ace-n addressed the comments, thanks!

Copy link
Member

@jmdobry jmdobry left a comment

Choose a reason for hiding this comment

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

vision/system-test/quickstart.test.js needs to be updated to use labelDetection instead of detectLabels: https://github.com/GoogleCloudPlatform/nodejs-docs-samples/blob/master/vision/system-test/quickstart.test.js

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@codecov-io
Copy link

Codecov Report

Merging #434 into master will increase coverage by 70.28%.
The diff coverage is 94.82%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #434       +/-   ##
===========================================
+ Coverage   13.55%   83.84%   +70.28%     
===========================================
  Files           3        4        +1     
  Lines         413      421        +8     
===========================================
+ Hits           56      353      +297     
+ Misses        357       68      -289
Impacted Files Coverage Δ
vision/detect.js 90.61% <100%> (+67.74%) ⬆️
vision/quickstart.js 88.88% <100%> (ø)
vision/textDetection.js 74.46% <84%> (+71.59%) ⬆️
vision/faceDetection.js 81.03% <90%> (+79.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7be652...c150b77. Read the comment docs.

@gguuss
Copy link
Contributor Author

gguuss commented Jul 31, 2017

I signed it!

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.

5 participants