Fallback to default clauses when msgstrs are empty #56
Conversation
| # it's empty, not generating this clause means that the dynamic clause will | ||
| # be executed, returning `{:default, msgid}` (with interpolation and so on). | ||
| if msgstr == "" do | ||
| nil |
There was a problem hiding this comment.
Maybe just omit this branch?
There was a problem hiding this comment.
@lexmag I'm not a fan of implicit nils for if actually :( I know you are (it's in the styleguide 😄) but I think that often relying on the default nil is a very implicit behaviour which makes it harder to understand what the code is doing. I think here this may be even stronger, because we're relying on the fact that nil is a valid quoted term that in this instance means "don't generate anything"; it's already slightly explicit this way, I'm afraid omitting the nil branch would only make it worse :| wdyt?
There was a problem hiding this comment.
@whatyouhide I could see the value of explicit nil if we're interested in the returning value (e.g. assignment), but when if has the meaning of skipping some work I believe it is much more clear without nil. 
(Personally, I unfold the diff above this change to see – maybe we are doing something with this nil :|).
There was a problem hiding this comment.
@lexmag I pushed a compromise, there's just one if (or unless) branch and the nil is implicit but I mention we're sometimes returning nil in the comments 😄 wdyt?
There was a problem hiding this comment.
Nice. 💛
(however this comment feels superfluous as it is just how if works ¯_(ツ)/¯)_
There was a problem hiding this comment.
@lexmag I know, I know 😄 but I value prose and explicitness too much to let this go 😸 jokes aside, I can remove the (by returning nil from this if) if you want :)
whatyouhide
force-pushed
the
empty-msgstr
branch
from
072bdb3
to
c51c89d
Compare
Before this commit, empty msgstrs were treated like any other
msgstr. This resulted in a nasty behaviour when generating new PO
files (from POT files) with e.g. `mix gettext.merge`, because empty
msgstrs were being returned.
This commit changes the behaviour of the Gettext.Compiler, which now
doesn't generate a function clause for translations with an empty msgstr
at all. This means such translations will fallback to the dynamic
clauses of `lgettext` and `lngettext`, which just return the
msgid (after interpolation) in a `{:default, _}` tuple. Plural
translations are treated this way if they have one or more empty
msgstrs.
whatyouhide
force-pushed
the
empty-msgstr
branch
from
c51c89d
to
0eef20c
Compare
|
❤️ 💚 💙 💛 💜 |
|
Let's ship it and see if there is anything left before we do a new release. :D |
Fallback to default clauses when msgstrs are empty
Should close #55.
Before this PR, empty msgstrs were treated like any other msgstr. This resulted in a nasty behaviour when generating new PO files (from POT files) with e.g.
mix gettext.merge, because emptymsgstrs were being returned.
This PR changes the behaviour of the
Gettext.Compiler, which now doesn't generate a function clause for translations with an empty msgstr at all. This means such translations will fallback to the dynamic clauses oflgettextandlngettext, which just return the msgid (after interpolation) in a{:default, _}tuple. Plural translations are treated this way if they have one or more empty msgstrs./cc @chrismccord and @endersstocker