Only deep symbolize keys when needed #2
Conversation
paarthmadan
force-pushed
the
pm/only-deep-symbolize-keys-when-needed
branch
from
616cf0a
to
820be73
Compare
lib/i18n/backend/base.rb
Outdated
| data = send(:"load_#{type}", filename) | ||
| unless data.is_a?(Hash) | ||
| locale_data = send(:"load_#{type}", filename) | ||
| unless locale_data.__getobj__.is_a?(Hash) |
There was a problem hiding this comment.
Not a huge fan of using a decorator here, especially if it creates the need for this kind of workaround, I hardly see it accepted upstream.
Have you considered returning two values from load_<type>? return translations, symbol_keys?.
Alternatively you could do something like locale_data.keys.first.is_a?(Symbol), it's not perfect that mixed type hash may break, but it's a decent heuristic.
There was a problem hiding this comment.
Not a huge fan of using a decorator here
Fair, this was my main apprehension, thanks for the feedback.
Have you considered returning two values
My only argument for not returning two values was to maintain the existing method signature that a "hash" (in this case a decorator over Hash) is returned. I assumed changing this would have more effect on the code / tests, but the scope seems quite small, so I've gone with that.
could do something like locale_data.keys.first.is_a?(Symbol)
I thought about that heuristic, but my evaluation was that it seems quite brittle. For instance, in tests, the first level of keys is a symbol, but deeper levels are strings. This wouldn't be a good heuristic in those cases. I suppose we could use a sampling approach to increase the probability, but I think the previous solution is better 👍
Addressed changes in 86f9919
There was a problem hiding this comment.
My only argument for not returning two values was to maintain the existing method signature that a "hash"
If upstream is picky about that, we can suggest putting a magic key in the top level hash instead.
When loading certain translations from file, they can be parsed into their Symbol representation. It is wasteful to traverse the entire object graph in these cases.
paarthmadan
force-pushed
the
pm/only-deep-symbolize-keys-when-needed
branch
from
86f9919
to
d7dd3dc
Compare
|
Closed in favour of ruby-i18n#588 |
This will be upstreamed to
ruby-i18n, but parking here to consolidate feedback for the approach. I'll move it upstream after we're internally aligned.Dependent on #1. When ruby-i18n#583 lands, I'll call
mark_keys_as_symbolizedin the YAML case, too.When loading certain translations from file, they can be parsed into their Symbol representation. It is wasteful to traverse the entire object graph in these cases.
I've introduce a decorator that is used in all the
load_*methods. It is used to encapsulate all the relevant information produced when loading translations. For now, it holds information about whether the loaded keys are already symbolized, which is used to configure a parameter instore_translations.