Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep custom flags with the custom_flags_to_keep option #370

Merged
merged 2 commits into from
Jul 31, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/gettext.ex
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,9 @@ defmodule Gettext do
have a matching reference. You can use this pattern to prevent Gettext from
removing messages that you have extracted using another tool.

* `:custom_flags_to_keep` - a list of custom flags that will be kept for
existing messages during a merge.
whatyouhide marked this conversation as resolved.
Show resolved Hide resolved

* `:write_reference_comments` - a boolean that specifies whether reference
comments should be written when outputting PO(T) files. If this is `false`,
reference comments will not be written when extracting messages or merging
Expand Down
45 changes: 30 additions & 15 deletions lib/gettext/merger.ex
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ defmodule Gettext.Merger do
by the references in the POT file

"""
@spec merge(Messages.t(), Messages.t(), String.t(), Keyword.t()) ::
@spec merge(Messages.t(), Messages.t(), String.t(), Keyword.t(), Keyword.t()) ::
{Messages.t(), map()}
def merge(%Messages{} = old, %Messages{} = new, locale, opts)
def merge(%Messages{} = old, %Messages{} = new, locale, opts, gettext_config)
when is_binary(locale) and is_list(opts) do
opts =
opts
Expand All @@ -61,7 +61,7 @@ defmodule Gettext.Merger do

stats = %{new: 0, exact_matches: 0, fuzzy_matches: 0, removed: 0, marked_as_obsolete: 0}

{messages, stats} = merge_messages(old.messages, new.messages, opts, stats)
{messages, stats} = merge_messages(old.messages, new.messages, opts, gettext_config, stats)

po = %Messages{
top_comments: old.top_comments,
Expand Down Expand Up @@ -99,10 +99,11 @@ defmodule Gettext.Merger do
end
end

defp merge_messages(old, new, opts, stats) do
defp merge_messages(old, new, opts, gettext_config, stats) do
fuzzy? = Keyword.fetch!(opts, :fuzzy)
fuzzy_threshold = Keyword.fetch!(opts, :fuzzy_threshold)
plural_forms = Keyword.fetch!(opts, :plural_forms)
custom_flags_to_keep = Keyword.get(gettext_config, :custom_flags_to_keep, [])

old = Map.new(old, &{Message.key(&1), &1})

Expand All @@ -114,7 +115,9 @@ defmodule Gettext.Merger do
case Map.fetch(old, key) do
{:ok, exact_match} ->
stats = update_in(stats_acc.exact_matches, &(&1 + 1))
{merge_two_messages(exact_match, message), {stats, Map.delete(unused, key)}}

{merge_two_messages(exact_match, message, custom_flags_to_keep),
{stats, Map.delete(unused, key)}}

:error when fuzzy? ->
case maybe_merge_fuzzy(message, old, key, fuzzy_threshold) do
Expand Down Expand Up @@ -202,37 +205,49 @@ defmodule Gettext.Merger do
# flags: we should take the new flags and preserve the fuzzy flag
# references: new contains the updated and most recent references

defp merge_two_messages(%Message.Singular{} = old, %Message.Singular{} = new) do
defp merge_two_messages(
%Message.Singular{} = old,
%Message.Singular{} = new,
custom_flags_to_keep
) do
%Message.Singular{
msgctxt: new.msgctxt,
msgid: new.msgid,
msgstr: old.msgstr,
comments: old.comments,
extracted_comments: new.extracted_comments,
flags: merge_flags(old, new),
flags: merge_flags(old, new, custom_flags_to_keep),
references: new.references
}
end

defp merge_two_messages(%Message.Plural{} = old, %Message.Plural{} = new) do
defp merge_two_messages(%Message.Plural{} = old, %Message.Plural{} = new, custom_flags_to_keep) do
%Message.Plural{
msgctxt: new.msgctxt,
msgid: new.msgid,
msgid_plural: new.msgid_plural,
msgstr: old.msgstr,
comments: old.comments,
extracted_comments: new.extracted_comments,
flags: merge_flags(old, new),
flags: merge_flags(old, new, custom_flags_to_keep),
references: new.references
}
end

defp merge_flags(old, new) do
if Message.has_flag?(old, "fuzzy") do
Message.append_flag(new, "fuzzy").flags
else
new.flags
end
@default_flags_to_keep ["elixir-format", "fuzzy"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only have a special rule for the fuzzy flag. The format flag is depending on the enabled interpolation module and might differ from elixir-format.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too late for merge, please still consider it @whatyouhide 😊

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maennchen ah that's a great call out. PR? 😬

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whatyouhide I recently had surgery and am on a lot of pain meds. I think it would be better if you could do this without me…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maennchen ah, sure! Good luck with recovery then 🤗

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! ✔️

defp merge_flags(old_message, new_message, custom_flags_to_keep) do
flags_to_keep = Enum.uniq(@default_flags_to_keep ++ custom_flags_to_keep)

%{flags: flags} =
Enum.reduce(flags_to_keep, new_message, fn flag, message ->
if Message.has_flag?(old_message, flag) do
Message.append_flag(message, flag)
else
message
end
end)

flags
end

@doc """
Expand Down
8 changes: 7 additions & 1 deletion lib/mix/tasks/gettext.merge.ex
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,13 @@ defmodule Mix.Tasks.Gettext.Merge do

defp merge_files(po_file, pot_file, locale, opts, gettext_config) do
{merged, stats} =
Merger.merge(PO.parse_file!(po_file), PO.parse_file!(pot_file), locale, opts)
Merger.merge(
PO.parse_file!(po_file),
PO.parse_file!(pot_file),
locale,
opts,
gettext_config
)

{merged
|> Merger.prune_references(gettext_config)
Expand Down
94 changes: 72 additions & 22 deletions test/gettext/merger_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ defmodule Gettext.MergerTest do
alias Gettext.Merger

@opts fuzzy: true, fuzzy_threshold: 0.8
@gettext_config []
@autogenerated_flags [["elixir-format"]]

describe "merge/2" do
describe "merge/5" do
test "headers from the old file are kept" do
old_po = %Messages{
headers: [~S(Language: it\n), ~S(My-Header: my-value\n)],
Expand All @@ -19,7 +20,7 @@ defmodule Gettext.MergerTest do

new_pot = %Messages{headers: ["foo"], messages: []}

assert {new_po, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {new_po, _stats} = Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)
assert new_po.headers == old_po.headers
end

Expand All @@ -34,7 +35,8 @@ defmodule Gettext.MergerTest do

new_pot = %Messages{messages: [%Message.Singular{msgid: "tomerge", msgstr: ""}]}

assert {%Messages{messages: [message]}, stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.msgid == "tomerge"
assert message.msgstr == "foo"
Expand Down Expand Up @@ -67,7 +69,13 @@ defmodule Gettext.MergerTest do
]
},
stats} =
Merger.merge(old_po, new_pot, "en", @opts ++ [on_obsolete: :mark_as_obsolete])
Merger.merge(
old_po,
new_pot,
"en",
@opts ++ [on_obsolete: :mark_as_obsolete],
@gettext_config
)

assert stats == %{
exact_matches: 1,
Expand All @@ -82,7 +90,8 @@ defmodule Gettext.MergerTest do
old_po = %Messages{messages: [%Message.Singular{msgid: "foo", msgstr: "bar"}]}
new_pot = %Messages{messages: [%Message.Singular{msgid: "foo", msgstr: ""}]}

assert {%Messages{messages: [message]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.msgid == "foo"
assert message.msgstr == "bar"
Expand All @@ -103,7 +112,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: [message]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.msgid == "foo"
assert message.comments == ["# existing comment"]
Expand All @@ -116,7 +126,8 @@ defmodule Gettext.MergerTest do

new_pot = %Messages{messages: [%Message.Singular{msgid: "foo"}]}

assert {%Messages{messages: [message]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert Message.has_flag?(message, "fuzzy")
end
Expand All @@ -137,7 +148,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: [message]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.extracted_comments == ["#. new comment"]
end
Expand All @@ -155,7 +167,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: [message]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.references == [{"bar.ex", 1}]
end
Expand All @@ -169,7 +182,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: [message]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.flags == @autogenerated_flags
end
Expand All @@ -194,7 +208,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: messages}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: messages}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert [
%Message.Singular{msgid: "foo", msgctxt: "context", msgstr: "with context"},
Expand Down Expand Up @@ -226,7 +241,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: [message]}, stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert %{exact_matches: 0, fuzzy_matches: 1, new: 0, removed: 0} = stats

Expand All @@ -243,7 +259,8 @@ defmodule Gettext.MergerTest do
old_po,
new_pot,
"en",
@opts ++ [store_previous_message_on_fuzzy_match: true]
@opts ++ [store_previous_message_on_fuzzy_match: true],
@gettext_config
)

assert message.previous_messages == [old_message]
Expand All @@ -263,7 +280,8 @@ defmodule Gettext.MergerTest do

# Let's check that the "hello worlds!" message is discarded even if it's
# a fuzzy match for "hello world!".
assert {%Messages{messages: [message]}, stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

refute Message.has_flag?(message, "fuzzy")
assert message.msgid == ["hello world!"]
Expand All @@ -287,7 +305,7 @@ defmodule Gettext.MergerTest do
}

assert {%Messages{messages: [message_1, message_2]}, stats} =
Merger.merge(old_po, new_pot, "en", @opts)
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message_1.msgid == ["hello world"]
assert message_1.msgstr == ["foo"]
Expand Down Expand Up @@ -316,7 +334,7 @@ defmodule Gettext.MergerTest do
}

assert {%Messages{messages: [message_1, message_2]}, stats} =
Merger.merge(old_po, new_pot, "en", @opts)
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message_1.msgid == ["hello world 1"]
assert message_1.msgstr == ["foo"]
Expand Down Expand Up @@ -350,7 +368,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: [message]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert Message.has_flag?(message, "fuzzy")
assert message.msgid == ["hello worlds!"]
Expand Down Expand Up @@ -381,7 +400,8 @@ defmodule Gettext.MergerTest do
]
}

assert {%Messages{messages: [message]}, stats} = Merger.merge(old_po, new_pot, "en", @opts)
assert {%Messages{messages: [message]}, stats} =
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert Message.has_flag?(message, "fuzzy")
assert message.msgid == ["Here is a cocoa ball."]
Expand Down Expand Up @@ -412,7 +432,7 @@ defmodule Gettext.MergerTest do
}

assert {%Messages{messages: [fuzzy_message, new_message]}, stats} =
Merger.merge(old_po, new_pot, "en", @opts)
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert %{exact_matches: 0, fuzzy_matches: 1, new: 1, removed: 0} = stats

Expand All @@ -439,7 +459,7 @@ defmodule Gettext.MergerTest do
}

assert {%Messages{messages: [message, plural_message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts)
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.msgid == "a"

Expand All @@ -464,7 +484,7 @@ defmodule Gettext.MergerTest do
stderr =
capture_io(:stderr, fn ->
assert {%Messages{messages: [message, plural_message]}, _stats} =
Merger.merge(old_po, new_pot, "en", @opts)
Merger.merge(old_po, new_pot, "en", @opts, @gettext_config)

assert message.msgid == "a"

Expand Down Expand Up @@ -492,7 +512,7 @@ defmodule Gettext.MergerTest do
stderr =
capture_io(:stderr, fn ->
assert {%Messages{messages: [message, plural_message]}, _stats} =
Merger.merge(old_po, new_pot, "en", opts)
Merger.merge(old_po, new_pot, "en", opts, @gettext_config)

assert message.msgid == "a"

Expand All @@ -504,6 +524,36 @@ defmodule Gettext.MergerTest do
assert stderr =~ "warning"
assert stderr =~ "The --plural-forms and :plural_forms options are deprecated"
end

test "custom flags defined by `custom_flag_to_keep` config are kept" do
whatyouhide marked this conversation as resolved.
Show resolved Hide resolved
old_po = %Messages{
messages: [
%Message.Singular{
msgid: "a",
flags: [["elixir-format", "fuzzy", "custom-flag", "other-custom-flag"]]
}
]
}

new_po = %Messages{
messages: [
%Message.Singular{
msgid: "a",
flags: [["elixir-format"]]
}
]
}

gettext_config = [custom_flags_to_keep: ["custom-flag"]]

{merged_message, _stats} = Merger.merge(old_po, new_po, "en", @opts, gettext_config)

assert %Messages{
messages: [
%Message.Singular{flags: [["elixir-format", "fuzzy", "custom-flag"]]}
]
} = merged_message
end
end

describe "prune_references/2" do
Expand Down