Refactor Input

[rogeriochaves]

This is part of #122

I believe it was really a good idea to take this approach, things are simpler now imo. Some problems I've found:

• Language was on inputs model and update, causing a need for it to sync with the language of the parent (reimbrusements), now only the view receives it when rendering
• Instead of the TranslationId, the already translated version of the label text was the one getting passed around, also causing the need to sync if language changed for example, or always translate when adding a label
• There was no types but the primitive ones, it was mostly String passing back and forth, which made me confused of the kind of values some functions were getting
• To know which field should be translated to what, the code was doing a correlation between 2 arrays, which led to the creation of some TranslationIds that were not fields
• No only inputs, but all other parts have its own Mdl model, Mdl msg and a case to update Mdl

What I did:

• Remove language and mdl from input
• Pass the parent (reimbrusements) model to the input view, so it can get language and mdl from there
• Removed Html.map so now this view produces Html Msg of the parent type
• Since now it is only doing that, I was able to rename it to Search, because in fact this part is only doing Search, and everything made more sense
• Transform the fields labels into an union type called Label
• Move mapping of translations to Fields file, so there is a direct mapping between a Label and a TranslationSet
• Create a Field type, which means a Label + Value, which is a String, but maybe we can improve that later (some fields need date, other number, etc, this could be described in the types)

So, for me, it looks like the code is simpler now, as Search doesn't have to manage translation and mdl, translation is only done at rendering time.

It is easier to understand, since we have less primitives and more types, we can look at function signature and understand what values it takes and what it do as not everything is a String, also creating more safety to the programmer. Then I've figured that not every value need to map to a string key, only the ones that will get on the url, the searchable ones.

Also, Inputs is too technical of a name, Search clarifies better what this part of the code do imo.

A few lessons learned:

• Don't save transformed data on the state together with the original one, they lead to sync problems, and your model ceases to be the single source of truth. For example, when we store the translated version together with the TranslationId, we start need to sync them. Instead, only save the original values on the state and transform them only when using, like when rendering. I've seend that happening on redux apps as well.
• Add new types when possible (but don't exaggerate!), you'll get more help and safety from the compiler, and more readability in the code
• Name things according to the domain, and think about functions reuse later, when you need
• Elm has the best refactor experience of all time, of all time!

What you think? I believe if you look commit by commit it is easier to understand the changes, let me know if you don't agree on some of those refactorings.

Cheers

[rogeriochaves]

TODO: fix tests :x

[coveralls]

Changes Unknown when pulling 223d493 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[coveralls]

Changes Unknown when pulling f596cf9 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[rogeriochaves]

Done, I haven't add any new tests, but at least I believe I haven't reduced coverage either

[cuducos]

Wow! This is way better than what we had. Many many thanks!

I added two minor inline comments, once we can rule them out this is good to merge!

[cuducos]

Do we really need to move translations out of the Internationalization module? I know Internationalization.elm is a long file that we could break down, but somehow having all translations in the same module is a good thing (even if we break this module down in submodules).

[rogeriochaves]

I agree, I scratched my head with the same point, but decided to give it a try to see how it looked. I too I like having them centralized, I'll move back.

[cuducos]

This comments were here because they are tests (elm-doc-test). Any special reason not to test the function with invalid query parameters?

Is there an implicit assumption that they mean no harm so I shouldn't be worried? That could be true, this might be just my OCD hahaha…

[rogeriochaves]

as fair as I remember, there was no way in the code that this function would have been called with an invalid value, or that wouldn't be a problem, I don't remember now. In fact, maybe we couldn't guarantee that just by looking at this function, maybe the ideal would be having an integration test.

But I'll look again and get back here.

[rogeriochaves]

So, I've changed the code to reduce confusion - or maybe cause more :x

I noticed that the toUrl function was only getting called when we had full control of the variables that were being passed to it, therefore we didn't need that isSearchable check. But that wasn't obvious neither guaranteed without an integration test, cause maybe another function could end up using this one, without passing the right params.

I also noted that toUrl was always getting called together with toQuery, we had to call one to then pass as argument to the other.

So what I did was moving the toUrl function to Search/Update.elm, since it is only used to build search urls, and now it take the fields as arguments, not query, and return a clean url, so it is no longer possible to pass weird params to it and get a malformed url.

And the page case is just a string appending

[cuducos]

69b91c0 🍾 ✨ Awesome ; )

[cuducos]

Done, I haven't add any new tests, but at least I believe I haven't reduced coverage either

The coverage just targets Python backend. I see little usage for coverage in Elm as a lot can be taken for granted just by the fact it compiled ; )

[rogeriochaves]

@cuducos yeah, I meant, the imaginary coverage for Elm hahahaha

[coveralls]

Changes Unknown when pulling 80c3fb4 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[coveralls]

Changes Unknown when pulling aa75264 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[coveralls]

Changes Unknown when pulling 203f210 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[coveralls]

Changes Unknown when pulling 203f210 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[coveralls]

Changes Unknown when pulling 6d9e509 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[coveralls]

Changes Unknown when pulling 6d9e509 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[coveralls]

Changes Unknown when pulling 69b91c0 on rogeriochaves:issue-122 into ** on datasciencebr:master**.

[cuducos]

I think that after 69b91c0 we're LGTM… or are you planning to refactor something else?

[rogeriochaves]

@cuducos yes, I am, but not in this branch, refactors are good to be done in pieces to avoid conflicts (:

merge it away!