Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The swap method is inconsistent and buggy #22

Open
christopherobin opened this issue Sep 30, 2013 · 4 comments
Open

The swap method is inconsistent and buggy #22

christopherobin opened this issue Sep 30, 2013 · 4 comments

Comments

@christopherobin
Copy link

When using the swap method between 2 elements of an array, the only way it works is by calling:
myArrayTome.swap(someKey, someValue) and it swaps the items at key someKey with the value someValue, but when trying to do myArrayTome.swap(someKey, someOtherKey) or myArrayTome.swap(someValue, someOtherValue) we start getting either exceptions or crashes.

Moreover, calling myValue.swap(someOtherValue) also crashes.

@bjornstar
Copy link
Contributor

There's only one documented way to do a swap.

Here's a quick example of using swap with an array:

var myArray = [ 'bread', 'bacon', 'lettuce', 'tomato', 'bread' ];

var myArrayTome = Tome.conjure(myArray);

myArrayTome.swap(1, myArrayTome[2]);

I can throw some more informative exceptions when the syntax is not matched if that will help.

@ronkorving
Copy link

Right. The swap method does seem to have fallback logic for the single argument case. The idea is nice, but doesn't actually work right now (as the code implies it should). Taking out this fallback mechanism would be an acceptable way to fix that I guess.

@christopherobin
Copy link
Author

It's just that when we tried to read the code with @ronkorving it seemed to support some other ways to do it. I guess yeah, removing the extra code (like when you only provide a key without target) and changing the errors would work nicely. Would be nice to support array.swap(key1, key2) too.
I can also write the fix if you don't have time to do it.

@antoniogs
Copy link

A array.swap(key1, key2) or array.swap(val1, val2) could be convenient so I pushed a swap modification than make as this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants