add_translation_dirs behaves the wrong way around

[rbu]

This is a follow-up to #1473. See the reproducer and notes there.

I would propose an API change that changes behaviour of add_translation_dirs to append specs at the end of existing specs instead of prepending it. That way, API users simply need to understand "last spec wins". This matches much closer to the mental model of "add".

It matches the current documented behaviour of how the method is to behave in case of multiple calls.

[piotr-dobrogost]

https://github.com/Kotti/Kotti which is based on Pyramid and meant as a framework calls its includeme() and then calls includeme() of all add-ons. Current behavior thus makes it impossible to override Kotti's own translations in add-on. Is the order of calling includeme() the same in other apps based on Pyramid and meant to be extensible? If so then I agree with @rbu's suggestion to change implementation so that last spec wins. Could we have acceptance for this API change from maintainers? includeme() is clearly meant to add or override declarations from earlier calls to other includeme()s Current behavior with regard to translations is directly opposite to this idea thus counter-intuitive and should be changed so that add_translation_dirs directive be in line with all other configuration directives.

It seems docstring with statements not in line with behavior was introduced in 5bf23fa by @mcdonc

[wichert]

-1 on anything that changes existing behaviour. There are far too many existing applications relying on the current behaviour, all of which would be broken by such a change.

[wichert]

FWIW I agree there is a problem here that should be solved, I just don't think we can make change existing behaviour. The simplest solution I can see is to add a parameter that to make add_translation_dirs add the new path at start of the search path. That should be a simple change to make.

[mmerickel]

I'm hesitant to change the existing behavior but on the other hand I think the current behavior is wrong and it would be weird for any apps to be relying on it.

[mmerickel]

@wichert can you say anything about that? Would it break any of your apps? I'm talking specifically about 2 consecutive calls to config.add_translation_dirs().

[mmerickel]

It seems to me this is simply about fixing the dirs to append instead of prepend as the last folder wins in an i18n search instead of the first folder.

Is something like the following remotely correct or am I misunderstanding this entirely?

diff --git a/pyramid/config/i18n.py b/pyramid/config/i18n.py
index 69af0f9..f8e5be9 100644
--- a/pyramid/config/i18n.py
+++ b/pyramid/config/i18n.py
@@ -73,7 +73,7 @@ class I18NConfiguratorMixin(object):
         directories = []
         introspectables = []

-        for spec in specs[::-1]: # reversed
+        for spec in specs:
             package_name, filename = self._split_spec(spec)
             if package_name is None: # absolute filename
                 directory = filename
@@ -93,15 +93,12 @@ class I18NConfiguratorMixin(object):
             directories.append(directory)

         def register():
-            for directory in directories:
-
-                tdirs = self.registry.queryUtility(ITranslationDirectories)
-                if tdirs is None:
-                    tdirs = []
-                    self.registry.registerUtility(tdirs,
-                                                  ITranslationDirectories)
-
-                tdirs.insert(0, directory)
+            tdirs = self.registry.queryUtility(ITranslationDirectories)
+            if tdirs is None:
+                tdirs = []
+                self.registry.registerUtility(tdirs,
+                                                ITranslationDirectories)
+            tdirs.extend(directories)

         self.action(None, register, introspectables=introspectables)

[wichert]

@mmerickel I have several apps in production which use a chain of PO files so a package can override (parts of) the translation from another package. Your proposed change would break those: the system would suddenly return the original translation instead of the app-specific override. It would mean that as part of migrating to a newer Pyramid version I will have to reverse the order of all add_translation_dirs() calls (some of which are indirect via config.include() to other packages).

I think there are two factors here: 1) in which order the list of directories passed to add_translation_dirs() is scanned, and 2) how successive calls to add_translation_dirs() are added to the search path. Changing 1) would break at least one of my apps, and not solve the real problem here. I can't easily tell if your proposed change does this or not, not knowing how spec is used. Fixing 2 by making a later add_translation_dirs() call add paths at the start of the lookup should address the problem, but has a risk of breaking apps (like mine) that currently use a specific order of config.include() and config.add_translation_dirs() to get the right translation search order.

[piotr-dobrogost]

@wichert

I think there are two factors here: 1) in which order the list of directories passed to add_translation_dirs() is scanned, and 2) how successive calls to add_translation_dirs() are added to the search path.

I think this is described in issue #1473, no?

[mmerickel]

@wichert I'm explicitly uninterested in breaking a single call to add_translation_dirs. I want to fix the issue with multiple calls.

At a high level pyramid's goal is to not enforce strict ordering on includes, but for certain things like "add to a search path" it's pretty much impossible without some sort of weighting.

[piotr-dobrogost]

@wichert

Fixing 2 by making a later add_translation_dirs() call add paths at the start of the lookup should address the problem

Actually fixing means putting them at the end not at the start. Michael is right here (emphasis mine):

It seems to me this is simply about fixing the dirs to append instead of prepend as the last folder wins in an i18n search instead of the first folder.

As to

(...) but has a risk of breaking apps (like mine) that currently use a specific order of config.include() and config.add_translation_dirs() to get the right translation search order.

here too I agree with Michael:

(...) I think the current behavior is wrong and it would be weird for any apps to be relying on it.

[mmerickel]

I would accept a patch that reverses the order such that later add_translation_dirs calls cause their folders to be searched before earlier calls, especially if a new parameter is added to the signature to preserve bw-compat such as a default of prepend=False. This would mirror the config.add_jinja2_search_path API [1] as well, which seems sane.

[1] http://docs.pylonsproject.org/projects/pyramid-jinja2/en/latest/api.html#pyramid_jinja2.add_jinja2_search_path

[piotr-dobrogost]

I would accept a patch that reverses the order such that later add_translation_dirs calls cause their folders to be searched before earlier calls, especially if a new parameter is added to the signature to preserve bw-compat such as a default of prepend=False.

This is the wrong way around. You had it right here (emphasis mine):

It seems to me this is simply about fixing the dirs to append instead of prepend as the last folder wins in an i18n search instead of the first folder.

It's the opposite of the common search pattern where some list is being iterated and the first item found wins (like in PATH env var or jinja2's search path I guess). This is just how gettext works.
So this would be append=False not prepend=False. However I would suggest biting the bullet and making it append=True. Yes, it would break some apps but rightfully so.

[mmerickel]

I've resolved this via #2902. I decided to keep things bw-compatible since it was me doing the work... ya'll had over 2 years to submit a different PR so....... Anyway with override=True you should be able to get the behavior you desire.

[piotr-dobrogost]

Thank you @mmerickel for fixing this.

[rbu]

Thanks a lot for finally adding an option that fixes this behavior. I agree with @piotr-dobrogost about the default, but I respect the rights of the coder 😄

[piotr-dobrogost]

@rbu

If we manage to convince @mmerickel then our view will become his view so his rights will be intact :)

[mmerickel]

From #2902 (comment)

Conclusion; if overwrite=True breaks something it means it was already broken (by having needless overwrites not meant to overwrite).

This is simply not true. As @wichert has already said, he's depended on this particular ordering in apps in the past in order to get overrides to work.

That being said, obviously there is a legitimate concern that the API is not as you (or many people) probably expect although I've worked hard to improve the docstrings to make it more clear what the API actually does.

1. This new API gives you a way around it without immediately breaking bw compatibility with people who had no warning of a coming change.

2. If you care so much about switching the flag, then you may offer some PRs to issue warnings when anyone is relying on the default instead of manually specifying override=True or override=False, and under Pyramid's typical deprecation period we could consider switching the default to override=True in the future.

[piotr-dobrogost]

That being said, obviously there is a legitimate concern that the API is not as you (or many people) probably expect

Would it be possible to fix the API in 2.0?

[mmerickel]

I think that ship has sailed for now as 2.0 is already released.