Remove unused gettext-setup dependency #91
Conversation
bastelfreak
force-pushed
the
gettext-setup
branch
from
5957471
to
7d96c20
Compare
This patch removes the dependency to gettext-setup. My understanding is that this was a preparation to support multiple locales/languages within the whole puppet ecosystem. I also heard that this plan was cancelled. Also this gem has no locales. On the other side pulling gettext-setup in raises several issues. The puppet gem starts it's locale logic if the gem is present and that raises some errors, for example https://tickets.puppetlabs.com/browse/MODULES-6598 Another issue is that https://github.com/puppetlabs/gettext-setup-gem isn't maintained at all. The dependencies aren't up2date, no response to open PRs since 1.5 years. This is also blocking the current vox pupuli modulesync run.
bastelfreak
force-pushed
the
gettext-setup
branch
from
7d96c20
to
9bc3abb
Compare
This reverts commit 86db8fa. The gettext-setup gem causes some ugly errors on our side: * puppetlabs/forge-ruby#91 * voxpupuli/puppet-mosquitto#18 * https://tickets.puppetlabs.com/browse/MODULES-6598
|
did you spot any remaining |
| if proxy = env[:request][:proxy] | ||
| errmsg = _("Unable to connect to %{scheme}://%{host} (using proxy %{proxy}) (for request %{path_query})") % { | ||
| errmsg << ("Unable to connect to %{scheme}://%{host} (using proxy %{proxy}) (for request %{path_query})") % { |
There was a problem hiding this comment.
This doesn't look correct.
| errmsg << ("Unable to connect to %{scheme}://%{host} (using proxy %{proxy}) (for request %{path_query})") % { | |
| errmsg = ("Unable to connect to %{scheme}://%{host} (using proxy %{proxy}) (for request %{path_query})") % { |
Same below. Also, you shouldn't introduce the single errmsg variable above the loop.
Or you can append, but then you should do this:
| errmsg << ("Unable to connect to %{scheme}://%{host} (using proxy %{proxy}) (for request %{path_query})") % { | |
| errmsg << " (using proxy #{proxy})" |
And then also drop the other errmsg << in the else.
|
Hey forge team, could I get a review here? :) |
There was a problem hiding this comment.
Thank you very much, @bastelfreak, and our apologies for the delay here! This looks good to me - we'll do a release later this week.
|
I'm slightly surprised my comments from #91 (comment) weren't addressed. You will have a regression where the error message is essentially duplicated since it concats. |
|
Hi @ekohl - I'm out today but I do plan to do some more cleanup here before we release this week - I'll be making sure your comments are addressed |
|
Thanks very much to @bastelfreak and @ekohl for your efforts here - version 3.2.0 has been released |
This patch removes the dependency to gettext-setup. My understanding is
that this was a preparation to support multiple locales/languages within
the whole puppet ecosystem. I also heard that this plan was cancelled.
Also this gem has no locales. On the other side pulling gettext-setup in
raises several issues. The puppet gem starts it's locale logic if the
gem is present and that raises some errors, for example
https://tickets.puppetlabs.com/browse/MODULES-6598
Another issue is that https://github.com/puppetlabs/gettext-setup-gem
isn't maintained at all. The dependencies aren't up2date, no response
to open PRs since 1.5 years. This is also blocking the current vox
pupuli modulesync run.