Skip to content
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

[FIX] GoogleTranslate errors #6

Conversation

PauSentis
Copy link

Description

This PR addresses an issue with the Google Translate functionality in RailsI18nManager::GoogleTranslate module. The following fixes have been implemented:

  1. Dependency Inclusion: Added the missing require 'easy_translate' statement to ensure proper dependency utilization.
  2. Translation Method Refactor: Modified the translation method to properly store the translation result in a variable, fixing the previous error.

This fix ensures that the Google Translate functionality now operates as intended, allowing for accurate translations and resolving any previous errors.

@@ -1,5 +1,6 @@
module RailsI18nManager
module GoogleTranslate
require 'easy_translate'
Copy link
Owner

Choose a reason for hiding this comment

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

According to general ruby standards, require statements should be at the top of the file not within a class or method.

But I would prefer we do all of our dependency require statements in lib/rails_i18n_manager/engine.rb

@@ -13,7 +14,7 @@ def self.translate(text, from:, to:)
return nil
end

EasyTranslate.translate(text, from: from, to: to, key: api_key)
translation = EasyTranslate.translate(text, from: from, to: to, key: api_key)
Copy link
Owner

Choose a reason for hiding this comment

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

This looks fine for this PR. Though I wonder why we have some complexity in the statements below. They probably need to be refractored a bit.

if translation
  str = translation.to_s
  
  if str.present?

@westonganger
Copy link
Owner

I've made one comment to address. Would you also mind adding a changelog entry?

@westonganger
Copy link
Owner

Looks like we should add at least one passing spec for this class

@westonganger
Copy link
Owner

Thanks for these fixes however I've added a more complete solution including adding specs for this class. See #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants