ChristophP / elm-i18next

https://package.elm-lang.org/packages/ChristophP/elm-i18next/latest
BSD 3-Clause "New" or "Revised" License
66 stars 12 forks source link

Fallback functionality #28

Closed nathanPro closed 2 years ago

nathanPro commented 3 years ago

Hello!

I think you do not need the functions tf, trf and customTrf for offering Fallbacks in your API. All you need to do is to expose a function

fallback : Translations -> Translations -> Translations

which combines two translations into a single one. The user is then free to combine Translations, and may even implement tf like this

resolveList : List Translations -> Translations
resolveList = List.foldr fallback initialTranslations

tf : List Translations -> String -> String
tf = resolveList >> t

Since Translations are implemented as dict, the union function can simply call Dict.union, as in

fallback (Translations d) (Translations f) = Translations (union d f)

I got tf, trf and customTrf passing all the tests with this idea, and could make a PR which does not break the API. If you are interested, I could also make a PR without tf. trf and customTrf, but I would like a thumbs up before.

ChristophP commented 3 years ago

@nathanPro Wow, I love the idea. I wanna take a bit to think about it because I just read this when going to the bathroom in the middle of the night (shouldn't touch my phone then 😅)

But some initial feedback. 1) I think it's great to be able to combine translations (monoid alert 😃) rather than having to expose lots of fallback functions 2) Since order is important in applying fallbacks I wonder if it would be clearer to have the fallback function take a list of translations instead of just two separate translations(basically what you did with resolveList). Otherwise I fear that if someone tries to combine more than two, it could be easy to lose track of the correct order. What do you think?

  1. I think this would be great to first release as a new minor version and if there's good feedback and no problems we can actually remove try, trf, and customTrf in the next major version.

Still need to think about naming and some consistency stuff when I'm a bit more awake but thanks for proposing this great idea.

ChristophP commented 3 years ago

Hi @nathanPro happy new year to you. After sleeping on it still seems like a good idea to add what you proposed to the package. What are your thoughts on my points in the comment above?

nathanPro commented 3 years ago

@ChristophP happy new year to us!

@nathanPro Wow, I love the idea. I wanna take a bit to think about it because I just read this when going to the bathroom in the middle of the night (shouldn't touch my phone then sweat_smile)

But some initial feedback.

1. I think it's great to be able to combine translations (monoid alert smiley) rather than having to expose lots of fallback functions

2. Since order is important in applying fallbacks I wonder if it would be clearer to have the fallback function take a list of translations instead of just two separate translations(basically what you did with resolveList). Otherwise I fear that if someone tries to combine more than two, it could be easy to lose track of the correct order. What do you think?

It seems to me that the best route is to expose both functions. I agree that we should expose the resolveList (with a better name, as feedbackOrder): I actually got it wrong the first time I tried it as I used foldl. As both functions are clearly equivalent (i.e., can be defined in terms of the other) I think it is reasonable to expose both of them, and we make part of our invariants that they are equivalent: maybe even add some fuzz tests that check if both functions define the same behavior.

1. I think this would be great to first release as a new minor version and if there's good feedback and no problems we can actually remove try, trf, and customTrf in the next major version.

Still need to think about naming and some consistency stuff when I'm a bit more awake but thanks for proposing this great idea.

So, should I send a PR exposing feedback and feedbackOrder?

ChristophP commented 3 years ago

Hi @nathanPro , the more I think about it the more I discovered unanswered questions about how certain things would behave if we follow through on our plan to expose something like a fallback function that in essence batches Translations into one?

Here are my thoughts:

Do you see the problem I am seeing? I have to admit that at first I was a bit blinded by eleganty batching translations together, but upon deeper reflection I fear we change the meaning of Translations too much to be practical? What do you think?

ChristophP commented 3 years ago

@nathanPro Ok, I just looked at your forked repo and realized I had a very different understanding of what you were trying to achieve. That's what I get for being half asleep when reading issues. Sorry for that. :sweat_smile: I actually thought you were proposing to make Translation to be some kind of monoid which can be concatenated by using fallback. In my mind that meant changing the implementation to

type Translation = WithoutFallback Dict String String | WithFallback (List (Dict String String))

which would've complicated the whole thing a lot. Hence my confusing previous comment.

Now I understand that you want to simply take advantage of the fact that Dict's can be unionized which is convenient for fallback languages. This is of course a whole different story and does not bring about all these issues that I mentioned in my previous comment. :facepalm:

ChristophP commented 3 years ago

So to answer your original question before I went off on my confusing tangent: Yes, a PR would be lovely :-D I'll also check out your other issue, but let's address them as separate PRs.

nathanPro commented 3 years ago

While I understand that you were much more convinced by the idea after seeing an implementation, I think that our discussion about the API is still valid.

I think that the current API (not the current implementation) implicitly introduces a monoid, and this seems to be good reason to make this monoid explicit. Regardless of the implementation (which we may decide to change when working on #29, or for any other reason), we have a type (i.e. Translations) with an associative operation (fallback) and a neutral element (emptyTranslations).

Moreover, I think we already have this monoid implicit in the API.

Having functions with the same signature, but with one version accepting a List Translations and the other accepting Translations are an example of treating a list of Translations as a Translations. With this idea in mind, we are actually defining an operation such that:

tf [a, b] = t (fallback a b)
trf [a, b] = tr (fallback a b)
customTrf [a, b] = customTr (fallback a b)

for every Translations a and b. These equations both explain how to be gone with tf, trf, customTrf, but also specify the behavior of fallback in terms of the current API.

Looking back at the questions you raised:

What would the output of hasKey be? [ ... ] Or the output of keys?

As I want keys (fallback a b) to be a description of the behavior of tf [a,b], it must output the union of keys a with keys b. This also specifies the behavior hasKey: hasKey k (feedback a b) is true if either hasKey k a or hasKey k b is true.

  • I've seen your other issue #29 . What would you get if you called toList on a Translation type with fallbacks? What would happen if you call insert? Do you insert into all of them?

I think the behavior of insert relies on a decision on what the abstraction Translations means. Regardless of the abstraction, it must be related to fallback.

For example, if we think of Translations as a dictionary which translates key to values, we may define a function f which builds a Translations with a given key value pair. In this context, we could define insert as

insert key value t = fallback (f key value) t

Similarly, if we think of Translations as something encoding the hierarchy present in the Json, then insert should accept a path and a value, and we could define a function g which builds a Translations with a given nesting and a single value. In this context, insert must respect

insert path value t = fallback (g path value) t

Does that make sense? I think we have good reason to be confident this is a good API change, even before deciding on #29 or other future issues.

ChristophP commented 3 years ago

Hi @nathanPro ,

I think your reasoning for the monoid behavior makes a lot of sense and I thing we should go ahead and implement the fallback using Dict.union like you proposed. This should produce expected behavior for hasKey and the other functions.

Regarding insert: I think we should mirror the API of t, where we already use a dot-separated string for keys, where dots indicate the nesting. To be consistent with that I think that insert should also use a dot-separated key for insertion, where dot indicates nesting. but we should move that part of the discussion over to #29

Other than that, do you have enough information to provide the PR for the fallback additions?

ChristophP commented 2 years ago

Since this is pretty old and got not responses in over a year I am closing this. Please reopen when it becomes relevant again.