Skip to content

Conversation

@dizcology
Copy link
Member

This PR updates the sample codes to support video intelligence v1beta2.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2017
@theacodes
Copy link
Contributor

LGTM, but please get travis to pass. Ping me when it's ready to merge.

@dizcology
Copy link
Member Author

Sorry should have pointed it out before you review it, there are three other functions (and the tests) that need to be updated. I will ping when the full PR is ready.

@dizcology
Copy link
Member Author

Do not merge until client library published and tested.

Copy link

Choose a reason for hiding this comment

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

Nit: should the i be lower case in EXPLiCIT?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a pretty long snippet, is that intended ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed - we often group the basic use cases of different methods into one script.

@dizcology dizcology requested a review from andrewsg September 18, 2017 03:20
Copy link
Member

Choose a reason for hiding this comment

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

We're sure the same video still works for these apparently different analyses?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was a rebranding of save_search_detection --> explicit_content_detection.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I thought the "safe search" analysis was "is this safe?", the inverse of the "explicit content analysis". If it's that confusing, no wonder they rebranded it.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah indeed it was confusing - safe_search_detection returned things which are not safe, for the user to filter out portions of the video, for example.

Copy link
Member

Choose a reason for hiding this comment

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

Should this be "start_time_offset.nanos"?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, thanks! i will scan through the other files and correct them.

Copy link
Member

Choose a reason for hiding this comment

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

same here, this should be start_time_offset

Copy link
Member

Choose a reason for hiding this comment

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

As above

Copy link
Member

Choose a reason for hiding this comment

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

As above. Might be worth making an offset_to_seconds helper function?

Copy link
Member Author

Choose a reason for hiding this comment

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

that would make a lot of sense, but since we include code snippets in the devsite, refactoring out a helper function means that the user cannot use the code copied off the devsite unless they also locate and copy the helper functions.

@dizcology
Copy link
Member Author

tests passed against the updated client library.

@theacodes theacodes merged commit a75e025 into GoogleCloudPlatform:master Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants