Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Language API changes (in progress) - DO NOT MERGE before Nov 15 #593

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

Merged
merged 14 commits into from
Nov 15, 2016
Merged

Conversation

puneith
Copy link
Contributor

@puneith puneith commented Oct 19, 2016

No description provided.

@puneith puneith self-assigned this Oct 19, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 19, 2016
# ['https://www.googleapis.com/auth/cloud-platform'])
# http = httplib2.Http()
# scoped_credentials.authorize(http)
# return discovery.build('language', 'v1', http=http)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI you can still do this. Just add the parameter discoveryServiceUrl:

return discovery.build(
    'language', 'v1', http=http,
    discoveryServiceUrl='https://{api}.googleapis.com/$discovery/rest?version={apiVersion}&labels=GOOGLE_INTERNAL')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's cool. Changes made.

@puneith puneith added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 20, 2016
@puneith puneith changed the title Nl15 Language API changes (in progress) - DO NOT MERGE Oct 20, 2016
@puneith puneith changed the title Language API changes (in progress) - DO NOT MERGE Language API changes (in progress) - DO NOT MERGE before Nov 15 Oct 20, 2016
@puneith
Copy link
Contributor Author

puneith commented Nov 10, 2016

@jerjou @jonparrott Please review.

Copy link
Contributor

@jerjou jerjou left a comment

Choose a reason for hiding this comment

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

It looks like there's a conflict with the README? Didn't Jon change all the READMEs recently?

@@ -25,13 +25,20 @@
from oauth2client.client import GoogleCredentials


# TODO REMOVE - when discovery is public
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet.

@@ -38,7 +38,9 @@ where `<command>` is one of: `entities`, `sentiment`, or `syntax`.

The script will write to STDOUT the json returned from the API for the requested feature.

For example, if you run:
Copy link
Contributor

Choose a reason for hiding this comment

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

I've recently updated all the READMEs to be autogenerated. Can you rebase?

def get_service():
credentials = GoogleCredentials.get_application_default()
scoped_credentials = credentials.create_scoped(
['https://www.googleapis.com/auth/cloud-platform'])
http = httplib2.Http()
scoped_credentials.authorize(http)
return discovery.build('language', 'v1beta1', http=http)
return discovery.build('language', 'v1',
http=http,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use credentials=credentials instead of http=http?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not until discovery is public. I have added a TODO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried recently? I think there was a change to the api client recently (possibly made by Jon) that retried getting the discovery doc if it gets a permission-denied error.

Copy link
Contributor Author

@puneith puneith Nov 10, 2016

Choose a reason for hiding this comment

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

Yes, it didn't work. Actually retried before making the comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh well :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

break at opening ( to avoid hanging indents. (I should make this into a t-shirt)

@theacodes
Copy link
Contributor

That bug is still open. :P

On Thu, Nov 10, 2016 at 1:48 PM Jerjou notifications@github.com wrote:

@jerjou commented on this pull request.

In language/api/analyze.py
#593:

def get_service():
credentials = GoogleCredentials.get_application_default()
scoped_credentials = credentials.create_scoped(
['https://www.googleapis.com/auth/cloud-platform'])
http = httplib2.Http()
scoped_credentials.authorize(http)

  • return discovery.build('language', 'v1beta1', http=http)
  • return discovery.build('language', 'v1',
  •                       http=http,
    

Oh well :-/


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#593, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAPUcyxAochUXDHwXTzL7ZF3_SbaGrdbks5q85E4gaJpZM4KbYUz
.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Nov 12, 2016
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Nov 12, 2016
@puneith
Copy link
Contributor Author

puneith commented Nov 14, 2016

@jonparrott discovery public and pending changes made.

@puneith puneith removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. In Progress labels Nov 14, 2016
@theacodes theacodes merged commit a236a5a into master Nov 15, 2016
@theacodes theacodes deleted the nl15 branch November 15, 2016 01:09
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.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.