-
Notifications
You must be signed in to change notification settings - Fork 139
Conversation
Cool, and props to all for making sure the code is as friendly as it can be. 🍻 The one thing I'm not sure about with the code in this PR is how it rewrites the request object temporarily; only to be changed a few lines deeper. I think our original implementation could probably still work: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/d511c762b625ed54e12c8da3e4e9ed0be083a779/packages/vision/src/index.js#L1719. It even supports |
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.
Need to npm run prettier
if you haven't already.
src/helpers.js
Outdated
let coerceRequest = (request, callback) => { | ||
// If request is not an object, then it is a specification of an image | ||
// in raw form; coerce it to an object. | ||
if (is.string(request) === true) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/helpers.js
Outdated
// If the file is specified as a filename and exists on disk, read it | ||
// and coerce it into the base64 content. | ||
if (request.image.source && request.image.source.filename) { | ||
fs.readFile(request.image.source.filename, {encoding: 'base64'}, (err, blob) => { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Yeah, I was more wanting to run the concept past stakeholders. |
lgtm apart from my requested changes |
Okay, I will test (and write tests) tomorrow morning. |
@jmdobry @stephenplusplus This is now ready for review. System tests pass locally. |
I looked at the previous code but decided to leave it as is. The existing code supports an extra case that the previous code did not (specifying the buffer in the 0.12.0 style). I do agree that the former support for a |
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.
Could you also update samples/quickstart.js
and samples/detect.js
to use the simpler interface?
src/helpers.js
Outdated
* @returns An object representing an AnnotateImageRequest. | ||
*/ | ||
let _requestToObject = request => { | ||
if (is.string(request) === true) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/helpers.js
Outdated
if (request.image.source && request.image.source.filename) { | ||
fs.readFile( | ||
request.image.source.filename, | ||
{encoding: null}, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Hey @jgeewax, @stephenplusplus, @jmdobry, (cc: @remi)
Background: JJ pinged me a couple weeks ago asking about the breaking changes between Vision 0.11.6 and 0.12.0. Essentially, we went from "we accept a string with the URL" to "send a big object". In the change moving to 0.12.0, our single-feature methods take the image object, but as part of GCN #2553 and GCN #2555, we actually are moving it to having to send the entire request structure:
The reason why we were doing this was to alter the request structure as little as possible to ensure forward compatibility. JJ asked, essentially, "For the simplest of cases...you only want to specify an image, why can you not detect non-objects and convert appropriately?" For example:
I responded, honestly, "Because I did not think of that at the time." I was focused on modifying the request as little as possible and only stripping out the feature flag and moving it over to the method, but the proposal has a lot of value. It does still mean that if you want to specify anything else at all (e.g. the
imageContext
case), you have to send the fullAnnotateImageRequest
structure, but that is where we are landing anyway.We merged the (breaking) fix from GCN #2555 but never actually launched it, so when we launch this repo-migration, there will be a breaking change involved. JJ's proposed fix here is additive, but it would be nice if we could reduce randomization on users as much as possible. I need to put a bow on this release soon to accommodate the v1p1beta1 release.
I have whipped up what the code (and docs) would essentially need to be if we did this. Is this something everyone would be happy with? If so, I will get the appropriate tests and such in place.
Fixes #2.