Allow whitelisting of references #86

maufl · 2016-04-04T15:17:49Z

Adds an option to specify a pattern of references that should be preserved by Gettext.
As discussed in #85.

whatyouhide · 2016-04-04T22:12:14Z

lib/gettext/po/translations.ex

+  ## Example
+
+      config :gettext,
+        exlude_refs_from_pruging: "^web\/static\/.*"


Typo in _pruging and in exlude.

Also, not sure why you're escaping / given you're in a string; it would make sense in ~r//, but in strings you can just use ^web/static/.*. :)

whatyouhide · 2016-04-04T22:24:58Z

Hey @maufl, great job, this is definitely headed in the right direction. I left a first batch of comments, let me know if everything is clear :)

maufl · 2016-04-05T07:46:33Z

Argh, this damn pruging typo followed me everywhere, I thought I found all of them ...

I made some improvements, I think they cover all your suggestions.

whatyouhide · 2016-04-05T08:45:30Z

lib/gettext.ex

-      that matches this pattern. You can use this pattern to prevent Gettext from
-      removing translations that you have extracted using another tool, e.g. from
-      static resources (web/static).
+    * `:excluded_refs_from_purging` - a regex that is matched agains translation


agains -> agains

maufl · 2016-04-05T09:25:48Z

I love how all you suggestions make the code smaller and simpler.

whatyouhide · 2016-04-05T09:34:34Z

lib/gettext/po/translations.ex

@@ -42,7 +42,7 @@ defmodule Gettext.PO.Translations do
  ## Example

      config :gettext,
-        exclude_refs_from_purging: ~r/^web\/static\/.*/
+        exclude_refs_from_purging: ~r{^web/static/.*}


Maybe better to make this consistent with the rest of the regexes, ~r{^web/static/}.

whatyouhide · 2016-04-05T10:31:55Z

@maufl awesome. So, I've been thinking about this and I think we should move the impure logic of this up from where it is now. By impure logic, I mean the retrieving of the :exclude_refs_from_purging option from :gettext's environment. We should move that up at least to Gettext.Extractor.merge_template/2, e.g.:

def merge_template(existing, new) do
  protected_pattern = Application.get_env(:gettext, :exclude_refs_from_purging)
  ...
    PO.Translations.protected?(t, protected_pattern)

This way, protected?/2 becomes simpler to reason about and you only retrieve the pattern from the app config once (instead of once for each translation).

Also, I don't love the idea of having a "fallback" regex that never matches when the :exclude_refs_from_purging option is not provided; personally, I'd like more something along these lines:

protected_pattern = Application.get_env(:gettext, :exclude_refs_from_purging)

# in protected?/2
def protected?(_t, nil), do: false
def protected?(_t, %Regex{} = pattern), do: ...

Wdyt?

maufl · 2016-04-05T12:01:21Z

I agree that it's ugly to fetch the pattern every time the protected?/1 method is called, but I'm not really sure if I like it better in merge_template/2.

Originally I used the fallback regex to avoid conditional expressions in protected?/1. Your proposal avoids conditional logic in in the function anyway. I think it's a better solution than what I did so far.

whatyouhide · 2016-04-05T12:05:05Z

@maufl why you don't like the idea of fetching the option in merge_template/2? Or better, why do you like it better in protected?/1?

(btw, ideally we would have this as up as possible, e.g., in the gettext.extract Mix task, that later passes this option around so that also merge_template/2 can take this option and become pure, but let's do small steps here)

maufl · 2016-04-05T12:09:56Z

I don't like it better in protected?/1, but I'm also not sure where I would put it :/ I'm lacking Elixir experience to have a good feeling about where it should go.

whatyouhide · 2016-04-05T12:11:59Z

@maufl let's put it in merge_template/2 and pass it along to protected?/2 for now, we can always revise later.

We're almost good to go :D We're only missing docs for this new option. You can describe this option in this section of the docs.

maufl · 2016-04-05T12:17:01Z

I already added docs for this option. Should be the first changeset when you compare the files. :)

whatyouhide · 2016-04-05T12:18:32Z

Ooops, very sorry :) I even left a few comments there 😄 So, very very last thing: can you squash all the commits in a single one? Then I think we're ready to go 👍

maufl · 2016-04-05T12:22:14Z

You mean, squash them locally and force push? I just discovered in another repo that GitHub allows you to squash commits of a pull request before merging them. If you hit "Merge pull request" you will get a "Confirm merge" button with a dropdown. The squash options is hidden in the dropdown.

whatyouhide · 2016-04-05T12:24:06Z

lib/gettext/po/translations.ex

+      iex> protected_pattern = ~r{^web/static/}
+      iex> t = %Gettext.PO.Translation{msgid: "Hello world!", references: [{"web/static/js/app.js", 42}]}
+      iex> Gettext.PO.Translations.protected?(t, protected_pattern)
+      true


Can we leave an empty line after the true? It's very common to do that in Elixir.

Of course we can :)

whatyouhide · 2016-04-05T12:25:04Z

lib/gettext/po/translations.ex

+  def protected?(_t, nil), do: false
+  def protected?(%{__struct__: s, references: []}, pattern) when is_translation(s), do: false
+  def protected?(%{__struct__: s, references: refs}, pattern) when is_translation(s),
+    do: Enum.any?(refs, fn({path, _}) -> Regex.match?(pattern, path) end)


This is great with do: instead of do ... end :) Last nitpick: can we always have the do: on a new line?

def protected?(_t, nil), do: false def protected?(%{__struct__: s, references: []}, pattern) when is_translation(s), do: false def protected?(%{__struct__: s, references: refs}, pattern) when is_translation(s), do: Enum.any?(refs, fn({path, _}) -> Regex.match?(pattern, path) end)

whatyouhide · 2016-04-05T12:25:19Z

oh, you're right, force of habit :) Left the superlast couple of comments, then we're ready to merge :)

maufl · 2016-04-05T12:29:04Z

Fixed :) Somehow Emacs Elixir mode has issues with this kind of indentation :/

whatyouhide · 2016-04-05T12:30:20Z

Wonderful 💟 Thanks so much!

(btw yep, this is hard code to indent I think, that's why elixir-mode has trouble with it)

maufl · 2016-04-05T12:32:40Z

Your welcome :) Feels great already to contribute to an Elixir library after only about 2 months.

maufl added 5 commits April 4, 2016 16:27

Enable excluding of translation refs from purging

5f59cab

Removed unused function

0e6e7b7

Add documentation

742c9f5

Add test for whitelisting

245f243

Add code sample to documentation

b078cde

whatyouhide reviewed Apr 4, 2016
View reviewed changes

maufl added 3 commits April 5, 2016 09:33

Fix typos

b5ddd84

Use a regex in the configuration instead of a string

2ff777a

Update the documentation

32a3c2f

whatyouhide reviewed Apr 5, 2016
View reviewed changes

maufl added 2 commits April 5, 2016 11:21

Small fixes in the docu

aa22b2a

Further simplify code

f989c64

whatyouhide reviewed Apr 5, 2016
View reviewed changes

Enforce consistency

061937b

Move configuration reading into merge_template/2

f80ac8c

whatyouhide reviewed Apr 5, 2016
View reviewed changes

Add empty line after code example

1bc9d01

whatyouhide reviewed Apr 5, 2016
View reviewed changes

Unify function declaration style

0876590

whatyouhide merged commit 784d97e into elixir-gettext:master Apr 5, 2016

whatyouhide mentioned this pull request Apr 5, 2016

Msgids are deleted from POT files #85

Closed

maufl deleted the allow-whitelisting branch April 5, 2016 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow whitelisting of references #86

Allow whitelisting of references #86

maufl commented Apr 4, 2016

whatyouhide Apr 4, 2016

whatyouhide Apr 4, 2016

whatyouhide commented Apr 4, 2016

maufl commented Apr 5, 2016

whatyouhide Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide Apr 5, 2016

maufl Apr 5, 2016

whatyouhide Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

Allow whitelisting of references #86

Allow whitelisting of references #86

Conversation

maufl commented Apr 4, 2016

whatyouhide Apr 4, 2016

Choose a reason for hiding this comment

whatyouhide Apr 4, 2016

Choose a reason for hiding this comment

whatyouhide commented Apr 4, 2016

maufl commented Apr 5, 2016

whatyouhide Apr 5, 2016

Choose a reason for hiding this comment

maufl commented Apr 5, 2016

whatyouhide Apr 5, 2016

Choose a reason for hiding this comment

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide Apr 5, 2016

Choose a reason for hiding this comment

maufl Apr 5, 2016

Choose a reason for hiding this comment

whatyouhide Apr 5, 2016

Choose a reason for hiding this comment

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016

whatyouhide commented Apr 5, 2016

maufl commented Apr 5, 2016