Add sort_by_msgid_case_insensitive
#326
Conversation
lib/gettext.ex
Outdated
| * `:sort_by_msgid_case_insensitive` - same as `:sort_by_msgid` but it | ||
| performs a case insensitive comparation when sorting alphabetically by msgid. |
There was a problem hiding this comment.
| * `:sort_by_msgid_case_insensitive` - same as `:sort_by_msgid` but it | |
| performs a case insensitive comparation when sorting alphabetically by msgid. | |
| * `:sort_by_msgid_case_insensitive` - a boolean that behaves similarly to | |
| `:sort_by_msgid`, but that sorts with a case-insensitive comparison. |
lib/gettext/extractor.ex
Outdated
| cond do | ||
| gettext_config[:sort_by_msgid] -> | ||
| Enum.sort_by(messages, & &1.msgid) | ||
|
|
||
| gettext_config[:sort_by_msgid_case_insensitive] -> | ||
| Enum.sort_by(messages, fn m -> Enum.map(m.msgid, &String.downcase/1) end) | ||
|
|
||
| true -> | ||
| messages |
There was a problem hiding this comment.
I think we should think about this API, because as it stands in this PR you could pass sort_by_msgid: true, sort_by_msgid_case_insensitive: false and the sorting would work, even if by reading the options I wouldn't really know what to expect.
What do you think about going with two complementary options, such as the existing :sort_by_msgid and :sort_is_case_sensitive? You'd have to specify sort_by_msgid: true, sort_is_case_sensitive: false in your use case.
There was a problem hiding this comment.
That's would be fine. In case you agree with that we could apply these changes in this PR.
lib/gettext/extractor.ex
Outdated
| cond do | ||
| gettext_config[:sort_by_msgid] -> | ||
| Enum.sort_by(messages, & &1.msgid) | ||
|
|
||
| gettext_config[:sort_by_msgid_case_insensitive] -> | ||
| Enum.sort_by(messages, fn m -> Enum.map(m.msgid, &String.downcase/1) end) | ||
|
|
||
| true -> | ||
| messages | ||
| end |
There was a problem hiding this comment.
To make sure that the line splitting of the message has no impact on the sorting, I would recommend changing this a bit:
| cond do | |
| gettext_config[:sort_by_msgid] -> | |
| Enum.sort_by(messages, & &1.msgid) | |
| gettext_config[:sort_by_msgid_case_insensitive] -> | |
| Enum.sort_by(messages, fn m -> Enum.map(m.msgid, &String.downcase/1) end) | |
| true -> | |
| messages | |
| end | |
| cond do | |
| gettext_config[:sort_by_msgid] -> | |
| Enum.sort_by(messages, &IO.chardata_to_string(&1.msgid)) | |
| gettext_config[:sort_by_msgid_case_insensitive] -> | |
| Enum.sort_by(messages, &(&1 |> IO.chardata_to_string() |> String.downcase())) | |
| true -> | |
| messages | |
| end |
There was a problem hiding this comment.
We were already sorting without IO.chardata_to_string/1. Do we know if it has ever caused issues? I think in general we should fix that in another PR and add test cases for it.
There was a problem hiding this comment.
@whatyouhide That’s something I should’ve probably done in #307 and spotted now when reading the changes.
I‘m not aware of issues so far but I can see how it could be an issue.
I can open a PR and add some tests if you prefer.
There was a problem hiding this comment.
Yeah, let's do that @maennchen, thanks 👍
whatyouhide
force-pushed
the
sort_by_msgid_case_insensitive
branch
from
4172d87
to
b40bd8a
Compare
|
@superruzafa I took the liberty of pushing a change to this branch that sets the |
Pull Request Test Coverage Report for Build b40bd8a5952648b85547215fbe20e82a51e756a0-PR-326
💛 - Coveralls |
Co-authored-by: Alfonso Ruzafa <alfonso.ruzafa@cabify.com>
Closes #325.