andrewplummer / Sugar

A Javascript library for working with native objects.
https://sugarjs.com/
MIT License
4.54k stars 305 forks source link

Make assign accept an array of objects #490

Closed alexandernst closed 7 years ago

alexandernst commented 9 years ago

The strings method assign could accept an array of object, making it more usefull.

Example:

people = [{name: 'John', age: 30}, {name: 'Susan', age: 26}, {name: 'Bob', age: 41}];
"Young people like {0:name} and {1:name}, who are {0:age} and {1:age} are younger than other people, like {2:name} who is {2:age}".assign(people);
alexandernst commented 9 years ago

As I need this for a project I'm working on, I'm willing to make a PR if such a feature will get accepted.

alexandernst commented 9 years ago

I patched SugarJS anyways as this is a must have for my project. Let me know if you need me to patch/do something more/else before you accept the patch, or if you'd rather reject the entire feature. If the second, I'll need to monkey-patch my version of SugarJS. @andrewplummer

alexandernst commented 9 years ago

Oh, btw, tests are failing but I really doubt it's because of my patch. Seems completely unrelated. Were the tests passing before?

andrewplummer commented 9 years ago

Although I can see a use for this I would very much like to avoid introducing new syntaxes to Sugar. Instead it makes more sense to perform a map operation on the array before passing it into assign, and this should be the preferred approach.

andrewplummer commented 9 years ago

Also yes I think the test failures should be unrelated

alexandernst commented 9 years ago

@andrewplummer I'm not quite sure to understand how that map operation would work at syntax level. Would you mind showing a simple example how you think the API should look like?

andrewplummer commented 9 years ago

Ah, so re-reading your example it seems Array#map might not be enough.. so maybe something like this:

var people = [{name: 'John', age: 30}, {name: 'Susan', age: 26}, {name: 'Bob', age: 41}];
var str = {};
people.forEach(function(p, i) {
    str['name' + i] = p.name;
    str['age' + i] = p.age;
});
"Young people like {name0} and {name1}, who are {age0} and {age1} are younger than other people, like {name2} who is {age2}".assign(str);
alexandernst commented 9 years ago

IMHO I really don't think that your suggestion is good. Your API suggestion forces creating a clone of the object that already exists (thus wasting memory and CPU) and making some operations (the forEach itself), when the hole point of Sugar is to just do things easily (tm).

Also, that way may introduce potential bugs. Take for example the following array:

var beer_pack = [{can1: 'xxx', 'can2': 'yyy', 'can3': 'zzz', ..... 'can11': 'kkk'}, {....}, {.....}];

The keys of can1 and can11 could mix.

Last but not less important, consider an array filled with a few objects that don't necessarily have the same keys.

Please reconsider my patch as it doesn't create any boilerplate, it doesn't waste any memory/cpu and the most important part, it doesn't break current Sugar API in any way, it just adds yet another way of using assign.

andrewplummer commented 9 years ago

I really don't agree at all with this. Sugar isn't intended to cover every possible use case, and when code is complex enough it requires the use of intermediary steps just like any other language or vanilla Javascript. CPU/Memory cycles aren't a compelling argument here as parsing out a custom syntax to walk the object could easily take just as many cycles. The first way is cleaner and more aligned with the spirit of assign.

Your second example is different and would likely require a different approach. If objects were to incorporate an id then that could be used to set up the string object to make unique keys that would allow a flat object that could then be passed to assign. There's nothing wrong whatsoever with this approach and it is preferable to adding an obtuse custom syntax. Assign is meant to be a simple method for string interpolation. I think what you really are looking for instead is a templating system.

I understand your argument, but bottom line Sugar is intended to help make operations easier but that does not mean it's there to allow any possible case to be handled in a single line. In this case setting up an intermediary object is much clearer as to intent.

alexandernst commented 9 years ago

So, in an hypothetical array such as:

[
    {'name': 'Bob', 'age': 25, 'car_1': 'Renault', 'car_2': 'BMW'}, 
    {'name': 'John', 'age': 30, 'car_1': 'Ford', 'car_2': 'Citroen'}
];

your final suggestion is to use some loop to create an object containing some hacky keys that would look like:

{
    'name_SugarPleaseDontCollide_1': 'Bob', 
    'age_SugarPleaseDontCollide_1': 25,
    'car_1_SugarPleaseDontCollide_1': 'Renault',
    'car_2_SugarPleaseDontCollide_1': 'BMW', 
    'name_SugarPleaseDontCollide_2': 'John',
    'age_SugarPleaseDontCollide_2': 30,
    'car_1_SugarPleaseDontCollide_2': 'Ford',
    'car_2_SugarPleaseDontCollide_2': 'Citroen'
}

hoping that _SugarPleaseDontCollide_i won't collide with anything? Or did I miss something?

andrewplummer commented 9 years ago

No. I'm saying that each specific case requires a specific solution that involves some common sense. If there is an id then maybe that could be used. If not, then use the index of an array like in my original example. Finally, if you actually have something like your contrived example of can11, there are a number of different ways you could handle this including creating unique ids (why would data this complex not have one to begin with?), using a delimiter like hyphens (your example doesn't really even collide anyway, it's just not pretty, which is why you would do something different), number padding, etc, etc. This is not as complex as you're making it seem.

In any case, if the data really is that complex then you should be using a sophisticated templating system long, long before this.

alexandernst commented 9 years ago

Feel free to ping me or reopen this if you change your mind. Monkey-patching will be until then.

andrewplummer commented 9 years ago

Thanks for posting though... I do understand your point and why you think assign could/should be improved. In the end it comes down to a question of balance, but for people with more specific use cases I think it's totally fine to roll their own methods; in fact part of Sugar's design is specifically for this purpose.

andrewplummer commented 9 years ago

OK! You are basically getting your wish here. I was opposed to making a special format just for assign, and also because it was 1-index based, which would have made things tricky. However this very nicely dovetailed with both issue #476 -- where assign will now become format and will be zero-index -- and also issue #472 -- which was requesting a way to get deeply nested namespaces with the dot operator. This of course supports arrays as well, so this should basically be exactly what you're looking for. To use your above example, the syntax will now be:

"Young people like {0.name} and {1.name}, who are {0.age} and {1.age} are younger than other people, like {2.name} who is {2.age}".format(people);

Hopefully this will work for you!

andrewplummer commented 7 years ago

This has gone out now! Hopefully it will provide what you need.