-
Notifications
You must be signed in to change notification settings - Fork 652
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
Upgrade to google-client-api 0.28.4 #14683
Conversation
418fc08
to
f733635
Compare
bcc8a36
to
b0a5a7f
Compare
b0a5a7f
to
2c0857c
Compare
cf99308
to
2826ad5
Compare
2826ad5
to
589bbd4
Compare
a77338d
to
114c5cb
Compare
5dcd85d
to
30bf12e
Compare
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.
Wow, more changes than I was expecting... Nice work! 💪
We should have more tests (coverage seems to be quite poor here) and code could be cleaner also (smaller methods, more descriptive variable names...), but I guess it would be to ask too much for an unexpected task like this 😅
We'll have to test it very carefully.
Don't be shy, ask for all changes needed. I've done it in a rush but that
is not excuse to have it cleaner or improved
El lun., 25 feb. 2019 19:18, Gonzalo Riestra <notifications@github.com>
escribió:
… ***@***.**** approved this pull request.
Wow, more changes than I was expecting... Nice work! 💪
We should have more tests (coverage seems to be quite poor here) and code
could be cleaner also (smaller methods, more descriptive variable
names...), but I guess it would be to ask too much for an unexpected task
like this 😅
We'll have to test it very carefully.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14683 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAtPeJOUHWnQm948DR8MTGVKWNWuzC4Xks5vRCjygaJpZM4bFsem>
.
|
I've tested it in the dedicated instance you have created, but I couldn't import any file from 2 different Google accounts, while those files do work in production. It says: |
3223989
to
45e9acc
Compare
Acceptance:
|
@gonzaloriestra I didn't answer properly to the test part. We can't have tests because this is a third party library use so we should test the integration but for that, we need to have an oauth app + authentication...and that is going to be open in the repository so... |
I've been testing this branch with Ruby 2.2 in a dedicated staging host. Everything's fine, except we'll need to wait for an hour to see if the synchronization updates correctly.
|
Synchronizations update correctly |
Related #14696
We need to upgrade it because it fails with ruby 2.4.x