(PUP-7518) i18n module loading by eputnam · Pull Request #5851 · puppetlabs/puppet

eputnam · 2017-05-10T00:33:27Z

This adds to Puppet::Module to "initialize" a module for translations. Each module must be added to FastGettext.translation_repositories, so that translations in the modules code will be available to Puppet. Two methods are added here, one to check if a module has been initialized and another to initialize. To initialize, we are just wrapping GettextSetup.initialize basically.

puppetcla · 2017-05-10T03:00:24Z

CLA signed by all contributors.

Iristyle · 2017-05-10T05:48:08Z

@eputnam the ticket here should be moved over to PUP, and commit message should be updated to correspond to the PUP ticket. We don't use MODULES tickets inside of Puppet.

Thanks!

Iristyle · 2017-05-10T05:49:23Z

Pinged @Magisus for a review as she's the resident gettext ninja.

eputnam · 2017-05-10T16:32:25Z

i may also add gettext config creation here in the future to reduce test clutter

glennsarti · 2017-05-10T16:30:10Z

lib/puppet/module.rb


    @absolute_path_to_manifests = Puppet::FileSystem::PathPattern.absolute(manifests)
+
+    # # i18n initialization for modules


nit: double hash

glennsarti · 2017-05-10T16:30:29Z

lib/puppet/module.rb

+      require 'locale'
+
+      initialize_i18n
+    rescue


It's nice to actually output the error message too

glennsarti · 2017-05-10T16:31:54Z

lib/puppet/module.rb

+    config_path = File.absolute_path('config.yaml', locales_path)
+    module_name = @forge_name.gsub("/","-") if @forge_name
+
+    nil if initialized_i18n? module_name


Should this be return nil?

explicit returns in Ruby are frowned upon?

Only at the ends of methods. This one is in the middle, and we want to quit if this is true.

glennsarti · 2017-05-10T16:32:31Z

lib/puppet/module.rb

+
+      initialize_i18n
+    rescue
+      Puppet.warning "GettextSetup initialization for #{@name} failed."


I assume this cannot be externalised as Gettext isn't setup yet.

So I think since this is going to use Puppet's GettextSetup and not the module's, it can be. And it should be. We are trying to externalize all warnings and errors.

good catch, it should have been but i guess we aren't externalizing yet.

glennsarti · 2017-05-10T16:35:02Z

spec/unit/module_spec.rb

+
+    before do
+      modroot = "#{modpath}/#{modname}/"
+      @config_path = "#{modroot}/locales/config.yaml"


nit: These variables should probably all be let definitions. None of them are used in before(:after) which is usually why @ variables are used.

glennsarti · 2017-05-10T16:37:58Z

lib/puppet/module.rb

+    end
+  end
+
+  def initialized_i18n? module_name


nit: It's probably more readable as i18n_initialized? e.g. "Is the door closed" vs "have you closed the door?"

+1 to name change

Magisus · 2017-05-10T16:40:50Z

lib/puppet/module.rb

+
+    begin
+      GettextSetup.initialize(locales_path) unless !File.file? config_path
+      Puppet.debug _("%{value0,} initialized for i18n: %{value1,}") % { value0: module_name, value1: GettextSetup.translation_repositories[module_name]}


We have been asked at least for now to not externalize debug messages.

Also for future reference, please try to replace the value0 etc. with meaningful names. This will make it easier for the translator. Also as written, the commas will cause a mismatch of keys and this will explode.

Magisus · 2017-05-10T16:45:47Z

lib/puppet/module.rb

+    end
+  end
+
+  def initialized_i18n? module_name


+1 to name change

Magisus · 2017-05-10T16:55:37Z

Are modules with externalized strings able to function normally if GettextSetup is not present? See https://github.com/puppetlabs/puppet/blob/master/lib/puppet.rb#L25-L45 for how we are handling this in Puppet.

eputnam · 2017-05-10T17:04:55Z

we haven't started externalizing yet fortunately. this is a great point @Magisus , thank you!

Magisus · 2017-05-30T22:53:32Z

This is blocked on more extensive testing.

Magisus · 2017-05-30T23:01:13Z

Also if this functionality is going to be needed/supported in the LTS, this needs to be retargeted at stable.

Magisus · 2017-06-16T18:45:30Z

Closing and reopening to fix CI

Magisus · 2017-06-22T21:08:26Z

Sorry to shuffle this again, but would you mind retargeting this at the 4.10.x branch? We're going to be deleting the stable branch in favor of that one soon.

eputnam · 2017-07-05T21:16:25Z

Would like to get this merged before the next release. @Iristyle have you had a chance to take a look?

joshcooper · 2017-07-10T23:29:52Z

Are the translations namespaced so that translations in the module(s) don't affect puppet, and vice-versa? Or do we need to rely/assume that the keys are unique across puppet and all modules?

eputnam · 2017-07-11T00:02:06Z

they are namespaced inside the master_domain by "project_name" from config.yaml so there should not be any interference.

Iristyle · 2017-07-11T15:12:14Z

lib/puppet/module.rb

+
  private

+  def i18n_initialized? module_name


nit: Please use parens in method declaration

Iristyle · 2017-07-11T15:14:28Z

lib/puppet/module.rb

    @absolute_path_to_manifests = Puppet::FileSystem::PathPattern.absolute(manifests)
+
+    # i18n initialization for modules
+    if Puppet::GETTEXT_AVAILABLE


GETTEXT_AVAILABLE as a top-level constant for a feature test feels a little weird. The ship has probably sailed on this decision, but wondering why this isn't in a Puppet feature?

Loading gettext happens very early (https://github.com/puppetlabs/puppet/blob/master/lib/puppet.rb#L51) and I seem to remember there being a reason we couldn't use Puppet features because of that. If that's not the case, this CAN probably change; the constant is only used internally.

Fair enough... that sounds like a plausible explanation.

Iristyle · 2017-07-11T15:17:44Z

lib/puppet/module.rb

+    config_path = File.absolute_path('config.yaml', locales_path)
+    module_name = @forge_name.gsub("/","-") if @forge_name
+
+    return nil if i18n_initialized? module_name


nit: can just return rather than return nil given the return value of initialize_i18n is not used

Do we want to guard against a nil value for module_name?
Also, no need to allocate a locales_path or config_path if not necessary - so method could be reordered a bit:

def initialize_i18n module_name = @forge_name.gsub("/","-") if @forge_name return if module_name.nil? || i18n_initialized?(module_name) locales_path = File.absolute_path('locales', path) begin GettextSetup.initialize(locales_path) Puppet.debug "#{module_name} initialized for i18n: #{GettextSetup.translation_repositories[module_name]}" rescue config_path = File.absolute_path('config.yaml', locales_path) Puppet.debug "Could not find locales configuration file for #{module_name} at #{config_path}. Skipping i18n initialization." end end

I'm also not sure how the method is designed to fail here? This method rescues ... and so does its caller. Can probably streamline the error-handling a bit.

it's not designed to fail, it just warns that there won't be any translations. i didn't think it was worth blocking a puppet run because translations aren't running. it's not affecting system configuration or anything, just the language of the log output, which will be obvious.

also, i like your version of the function a lot. thanks for the review!

eputnam · 2017-07-11T17:41:30Z

perhaps a little blurb above the initialize_i18n function is in order also?

Wraps GettextSetup.initialize which adds a translation_repository to the master text_domain for a module. For successful initialization, GettextSetup requires a locales/ dir and a locales/config.yaml file. If the module already has a translation_repository in the master text_domain or the module has no name, this function returns nil.

eputnam · 2017-07-12T18:02:46Z

doing some git shuffling now, don't merge yet.

eputnam · 2017-07-12T20:52:13Z

ok, canoodling over, ready for merge.

Magisus · 2017-07-12T21:29:28Z

I would go ahead and squash those last two commits. They're not really logically separable.

This adds to Puppet::Module to "initialize" a module for translations. Each module must be added to FastGettext.translation_repositories, so that translations in the modules code will be available to Puppet. Two methods are added here, one to check if a module has been initialized and another to initialize. To initialize, we are just wrapping GettextSetup.initialize basically.

Iristyle · 2017-07-14T21:06:34Z

lib/puppet/module.rb

+    # i18n initialization for modules
+    if Puppet::GETTEXT_AVAILABLE
+      begin
+        initialize_i18n


What I meant here @eputnam is that if initialize_i18n never raises an exception based on its code already catching inside a begin / rescue ... that means there's no need for a rescue here.

Merged for now, but this could probably be tightened up a little.

Iristyle requested a review from Magisus May 10, 2017 05:48

eputnam force-pushed the modulesi18n branch from cabf293 to 34731ba Compare May 10, 2017 16:22

eputnam changed the title ~~(MODULES-4502) i18n module loading~~ (PUP-7518) i18n module loading May 10, 2017

glennsarti reviewed May 10, 2017

View reviewed changes

Magisus reviewed May 10, 2017

View reviewed changes

Magisus added the blocked PRs blocked on work external to the PR itself label May 30, 2017

eputnam force-pushed the modulesi18n branch 3 times, most recently from 4fd3a5a to 238c010 Compare June 16, 2017 18:42

eputnam changed the base branch from master to stable June 16, 2017 18:42

eputnam removed the blocked PRs blocked on work external to the PR itself label Jun 16, 2017

Magisus closed this Jun 16, 2017

Magisus reopened this Jun 16, 2017

eputnam changed the base branch from stable to 4.10.x June 26, 2017 16:29

Magisus requested a review from Iristyle June 27, 2017 18:20

Iristyle reviewed Jul 11, 2017

View reviewed changes

eputnam force-pushed the modulesi18n branch 2 times, most recently from bd3565a to 6bb02e2 Compare July 12, 2017 17:33

eputnam force-pushed the modulesi18n branch 2 times, most recently from 3586606 to 9252b22 Compare July 12, 2017 19:59

eputnam force-pushed the modulesi18n branch from 9252b22 to 7e5dab5 Compare July 12, 2017 22:13

Iristyle merged commit d9d2edc into puppetlabs:4.10.x Jul 14, 2017

Iristyle reviewed Jul 14, 2017

View reviewed changes


		@absolute_path_to_manifests = Puppet::FileSystem::PathPattern.absolute(manifests)

		# # i18n initialization for modules

Conversation

eputnam commented May 10, 2017

Uh oh!

puppetcla commented May 10, 2017

Uh oh!

Iristyle commented May 10, 2017

Uh oh!

Iristyle commented May 10, 2017

Uh oh!

eputnam commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Magisus commented May 10, 2017

Uh oh!

eputnam commented May 10, 2017

Uh oh!

Magisus commented May 30, 2017

Uh oh!

Magisus commented May 30, 2017

Uh oh!

Magisus commented Jun 16, 2017

Uh oh!

Magisus commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eputnam commented Jul 5, 2017

Uh oh!

joshcooper commented Jul 10, 2017

Uh oh!

eputnam commented Jul 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iristyle Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Magisus Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iristyle Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Magisus commented Jun 22, 2017 •

edited

Loading

Iristyle Jul 11, 2017 •

edited

Loading

Magisus Jul 11, 2017 •

edited

Loading

Iristyle Jul 11, 2017 •

edited

Loading

Iristyle Jul 14, 2017 •

edited

Loading