Google translate #1978

joenas · 2017-04-20T21:44:56Z

Work on #1800.

It's more or less a copy paste of TranslationAgent. There was talk of implementing an agent that supports multiple backend but I haven't really the time for that, @Maroo-b feel free to have a look!

joenas · 2017-04-21T15:14:03Z

Should I squash the commits? Not entirely sure how to tbh because I never use it but I could probably work it out...

Maroo-b

@joenas great work :)

I added a note about upgrading the google-api-client gem, for the moment are we keeping the old translator agent ?

Regarding squashing the commit it's easy:

git stash : start by stashing files not related to the PR (config, or other non committed files)
git rebase -i HEAD~N : in our case the N would be 7 the number of commits to squash.
Inside the editor edit the action to 's' (for comments to squash) but leave the first one ( 'Added GoogleTranslationAgent' )
The editor will be opened again to rename the parent commit.
git push -f : you have to force pushing the PR to publish the changes.

I hope this will help :)

Maroo-b · 2017-04-23T08:32:17Z

Gemfile.lock

-      retriable (>= 1.4)
-      signet (>= 0.5.0)
-      uuidtools (>= 2.1.0)
+    google-api-client (0.9.28)


is there a reason to upgrade to google-api-client 0.9.28 from 0.7.1 ? because based on changelog the version 0.9 introduces breaking changes and we can check this guide for migration: link

I'm pretty sure there was but I can't remember it 😛 I'll have a look again and see if it works with the old one. Also, the specs passed so I figured it didn't matter.

Although if the spec for calendar or whatever it was is stubbed I reckon that won't help much..

I suggest to keep the same version if it works to avoid regression. :)

I agree keeping the old version would be better, otherwise we need to manually verify the calendar agent is still working.

I think the main reason was because the old version seems to be a pain to use and all the examples I could find for translate was for a newer version. I'll see if I can revert tho.

You are right, it looks like 0.9.11 was the first version to add the translation API.

joenas · 2017-04-24T09:41:35Z

for the moment are we keeping the old translator agent

Not sure how this is usually handled in this project, I guess it should be deprecated or at least a note added to it so that if someone tries to use it they'll know why it doesn't work!

Maroo-b · 2017-04-24T12:32:24Z

I guess it would be better to remove the old translator for the moment, maybe @cantino have another suggestion :)

dsander

Not sure how this is usually handled in this project, I guess it should be deprecated or at least a note added to it so that if someone tries to use it they'll know why it doesn't work!

If the old translation agent does not work anymore you can safely remove the Agent and its specs. The user will get a warning message in which the Agent can be deleted from the database. For an example we recently removed the BeeperAgent

dsander · 2017-04-24T13:15:47Z

spec/models/agents/google_translation_agent_spec.rb

+require 'rails_helper'
+
+describe Agents::GoogleTranslationAgent do
+    before do


We generally use two space indentation in ruby files, looks like your editor choose to use 4 here 😄

Its copy paste, I use 2 as well but for some reason that wasn't converted :P

dsander · 2017-04-24T13:16:22Z

Gemfile.lock

-      retriable (>= 1.4)
-      signet (>= 0.5.0)
-      uuidtools (>= 2.1.0)
+    google-api-client (0.9.28)


I agree keeping the old version would be better, otherwise we need to manually verify the calendar agent is still working.

dsander · 2017-04-24T13:20:35Z

app/models/agents/google_translation_agent.rb

+    end
+
+    def validate_options
+      unless ENV['GOOGLE_API_KEY'].present? && options['to'].present? && options['content'].present? && options['expected_receive_period_in_days'].present?


Would it make sense to move the API key to the Agent options? That way every user can configure it and it does not require a server restart.

You know I had the same thought when writing the spec and revisiting the TranslatorAgent, I guess I was just stuck in work routine always using ENV.

dsander · 2017-04-24T13:24:08Z

app/models/agents/google_translation_agent.rb

+          result = translate.list_translations(value, opts['to'], source: translate_from)
+          translated_event[key] = result.translations.last.translated_text
+        end
+        create_event :payload => translated_event


We generally use the new ruby hash syntax when possible: create_event payload: translated_event

Then I'm very happy to change it since I despise hash rockets hehe

joenas · 2017-04-25T08:59:36Z

Would it be alright if I added guard? It's such mess running specs manually :D

joenas · 2017-04-25T09:00:42Z

Lol nevermind, it's there already. Last time I tried I didn't have recent upstream.

No more hash rockets Added info to description Indentation

Maroo-b · 2017-04-25T10:32:37Z

app/models/agents/google_translation_agent.rb

+
+    def translate_service
+      @translate_service ||= Google::Apis::TranslateV2::TranslateService.new.tap {|service| service.key = options['google_api_key']}
+    end


I noticed that you memoize the method translate_service I agree it's better for performance but in our case the method has options['google_api_key'] as parameter. What do you think about using a hash with the value of options['google_api_key'] as keys in case a user changes the API key.

Ah it's only initialized once at startup? Then it's probably a good idea yeah.
Do you mind sharing an example of how you would do? I recently updated the code when downgrading the gem.

I think it should be fine, since the API key is not interpolated it can be safely memoized. The instance variable will only be set for one runtime of the Agent.

@dsander you are right, it will be memoized for an instance scope :)

joenas · 2017-04-25T11:04:48Z

Alright guys, I managed to get it to work with 0.7.1 with some intensive googling :D
@Maroo-b said something about not memoizing with the options hash, so if I could get an example I'll change that and then we're good to go?

dsander · 2017-04-25T11:29:36Z

@joenas Awesome! Can we get rid of the other gem version changes or are they required?

joenas · 2017-04-25T16:15:29Z

Can we get rid of the other gem version changes

Done!

cantino · 2017-04-25T20:56:18Z

Gemfile

@@ -47,8 +47,8 @@ gem 'hypdf', '~> 1.0.10'          # PDFInfoAgent
 # FIXME needs to loosen omniauth dependency
 gem 'weibo_2', github: 'dsander/weibo_2', branch: 'master'

-# GoogleCalendarPublishAgent
-gem "google-api-client", require: 'google/api_client'


Can you add back in the require: 'google/api_client' so that the GoogleCalendarPublishAgent will also work ok? I think you should also copy the gem_dependency_check and #{'## Include "google-api-client" in your Gemfile to use this Agent!' if dependencies_missing?} lines from GoogleCalendarPublishAgent into your Agent so that it correctly detects if google-api-client has been removed from the Gemfile, and remove the require line that you added at the top.

Done and done!

cantino · 2017-04-25T21:38:14Z

Looks great to me, nice work! Does anyone else have any other feedback before we merge?

knu

Looks great!

knu · 2017-04-26T00:27:02Z

app/models/agents/google_translation_agent.rb

+  class GoogleTranslationAgent < Agent
+    cannot_be_scheduled!
+
+    gem_dependency_check { defined?(Google) && defined?(Google::APIClient) }


defined?(Google) && is redundant here. defined?(Google::APIClient) won't err if Google is undefined.

Ah, good to know, thanks!

cantino · 2017-04-26T05:49:21Z

Thanks @joenas & @Maroo-b!

joenas · 2017-04-26T10:05:40Z

Btw @Maroo-b thanks for the squash instructions although I never got around to use it :D Next time!

joenas and others added 7 commits November 18, 2016 00:00

Added GoogleTranslationAgent

772662a

Well, I forgot to add it..

09aae9c

Wrong ruby version

584afdc

Merge with master

25c73b8

Removed duplicate GoogleTranslationAgent

3514ede

Fixed Gemfile upgrades by adding required version of jwt

dddd9fc

Added spec for GoogleTranslationAgent and fixed description

3042a38

Maroo-b reviewed Apr 23, 2017

View reviewed changes

dsander reviewed Apr 24, 2017

View reviewed changes

joenas added 2 commits April 25, 2017 11:40

PR fixes:

424d04b

No more hash rockets Added info to description Indentation

Deleted TranslationAgent because service is deprecated

5de4f1f

Maroo-b reviewed Apr 25, 2017

View reviewed changes

Reverted google-api-client version

6645376

Downgraded rest of gems and fixed hash rockets

61f14d1

cantino reviewed Apr 25, 2017

View reviewed changes

More PR fixes

c38f602

knu approved these changes Apr 26, 2017

View reviewed changes

cantino merged commit 2b41b6c into huginn:master Apr 26, 2017

joenas deleted the google_translate branch April 26, 2017 10:00

Google translate #1978

Google translate #1978

Conversation

joenas commented Apr 20, 2017

joenas commented Apr 21, 2017

Maroo-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joenas commented Apr 24, 2017

Maroo-b commented Apr 24, 2017

dsander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joenas commented Apr 25, 2017

joenas commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joenas commented Apr 25, 2017

dsander commented Apr 25, 2017

joenas commented Apr 25, 2017

cantino Apr 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cantino commented Apr 25, 2017

knu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cantino commented Apr 26, 2017

joenas commented Apr 26, 2017

cantino Apr 25, 2017 •

edited