tflori / angular-translator

translation module for angular
https://tflori.github.io/angular-translator/
MIT License
21 stars 6 forks source link

implement a search method for translator and use it in instant method #59

Closed tflori closed 7 years ago

tflori commented 7 years ago

this will solve #58

tflori commented 7 years ago

@BorntraegerMarc you see how this is affecting the interface: see the changes to translate and the return value from instant. I also need to adjust the return value of the observe method... And of course we need documentation for it...

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.03%) to 99.327% when pulling b750a0c40f33bc952bb413837aa626ecc37abb82 on feature-search into 8579fd83cc3f4ae61115ea4790b4fae662b612d0 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.03%) to 99.327% when pulling 21508d38ed6e2010addcff53d3af7b777c04c85d on feature-search into 8579fd83cc3f4ae61115ea4790b4fae662b612d0 on master.

tflori commented 7 years ago

@BorntraegerMarc for this "problem" we need to change the whole interface. currently it is typesafe already: you give an array of keys - you get an array of translations; you give a single key - you get a translation. With this proposal it changes to string or object - so it is not typesafe anymore.

So one thing is the methods that accepts string|string[] (observe, translate, instant). Unfortunately it's not like java where you write two times the same method name with different parameters and the app is using which fits best. For this they have to be changed to instant and instantArray, translate and translateArray, observe and observeArray.

The other thing is to search when a key is not found - this results to objects instead of string (either in an array or a single object). To pop this out we would need to add translateSearch and observeSearch.

For backward compatibility we can not remove string|string[] from translate, instant or observe. We can mark them deprecated and remove them in next major release. But we can already create the Array methods. And we can remove the search functionality from them when key is not found and create the Search methods.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-4.6%) to 94.728% when pulling 119bad881ab438377aad308120ed2bc0816a69a6 on feature-search into 8579fd83cc3f4ae61115ea4790b4fae662b612d0 on master.

BorntraegerMarc commented 7 years ago

@tflori I agree with every point you made except the part where you stated "so it is not typesafe anymore"

why wouldn't passing a interface or a TS pre determined object be typesafe? And I also don't really understand how passing a string is more typesafe?

if you call translate('trans.de') you can't do any check at compilation time if this object really exists or if it the right one. You always need to execute the code and check in the frontend if you definitely got the correct value.

With translate(IMPORTED_TRANS.trans.de) the compile will throw an error if an object does not exist.

BorntraegerMarc commented 7 years ago

But anyway: Just wanted to have a open discussion 😄 do you think we should create an issue and plan it for v3? Or you're not a fan of the idea at all?

tflori commented 7 years ago

@BorntraegerMarc I'm not sure if I understand you correct.. what do you mean to pass an interface or a TS object?

when you call translate('string') you get string back - always. when you call translate(['string']) you get array back - always.

I implemented now a different method and added a deprecation information in the translate, observe and instant method doc-comment. https://github.com/tflori/angular-translator/pull/59/commits/b750a0c40f33bc952bb413837aa626ecc37abb82

The functions need to be tested before I can merge it for version 2.3 but then you will be type safe - and we remove the |string[] from translate, observe and instant we are absolutley typesafe.

BorntraegerMarc commented 7 years ago

Well, to sum it up my suggestion would be to change the method translate(keys: string|string[], to translate(keys: any|any[],...

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.6%) to 95.673% when pulling de7dbcb8fe6374f75177431520767769ce14526a on feature-search into 8579fd83cc3f4ae61115ea4790b4fae662b612d0 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-2.2%) to 97.111% when pulling bcb807f708c2b8518913a492351a9ead002067dc on feature-search into 8579fd83cc3f4ae61115ea4790b4fae662b612d0 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.06%) to 99.358% when pulling 6a226ba62a15e9111d6f2a9f9c8a9cfc83bd0aaf on feature-search into 8579fd83cc3f4ae61115ea4790b4fae662b612d0 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.06%) to 99.358% when pulling 6a226ba62a15e9111d6f2a9f9c8a9cfc83bd0aaf on feature-search into 8579fd83cc3f4ae61115ea4790b4fae662b612d0 on master.