Return a sorted copy from array.sort()
#250
Conversation
tests/unit/array/sort-test.js
Outdated
| @@ -0,0 +1,83 @@ | |||
| import { sort } from 'ember-awesome-macros/array'; | |||
There was a problem hiding this comment.
Did you see the existing tests at https://github.com/kellyselden/ember-awesome-macros/blob/master/tests/integration/array/sort-test.js?
There was a problem hiding this comment.
Oh, no, totally missed those 🤦♂️ just saw the length tests in tests/unit/array and foolishly thought there weren't any for sort
There was a problem hiding this comment.
Would you like me to move the non-duplicate tests (the last three, I believe) into the existing integration test file? (they seemed a bit more like unit tests to me, but not sure how/where you're drawing the line?)
The composibility test was copied basically verbatim from length-test, and the other two are testing the specific functionality of this patch (ie they fail without the accompanying code change)
There was a problem hiding this comment.
The line I'm using is unit tests use mocking (I verify .sort() was called on my mock) and the integration tests use the real implementation more along the lines of a smoke test.
There was a problem hiding this comment.
ok, well in that case what i've written as unit tests are in fact integration tests...but half of what's currently in the integration tests file are unit tests :P
Edit : nvm, i was mis-reading -- they use stubs for the sort definitions, not the array's .sort()
I'll move mine over to the existing test file, and update the PR
There was a problem hiding this comment.
@kellyselden What are your thoughts on current behavior of returning undefined vs Ember.computed.sort behavior of returning [] when the source is undefined?
There was a problem hiding this comment.
This is an issue that most the macros have. I couldn't decide what the default value is if all the params are undefined, so most return undefined. I'm seeing that lodash returns empty arrays in most cases, so maybe all the macros need this change.
There was a problem hiding this comment.
Filed an issue for this #251
|
ok, moved the tests from |
|
Looks great. Feel free to squash, or I can do it via github UI. |
Current behavior is to sort the original source array Fixes kellyselden#249
gabrielgrant
force-pushed
the
sort-copy
branch
from
35159ce
to
fc9ce65
Compare
|
Cool, killed the redundant test commits, should be ready to merge (squashing further if you want) afaics |
|
Great work. Released v0.31.3. |
|
Sweet, thanks! |
Fixes #249
Note:
sort()currently returnsundefinedwhen the observed property is undefined, while Ember'scomputed.sort()returns an empty array. Not sure if this difference is intended, I found it a bit surprising.At the moment, it seems
array/-utils.normalizeArraywill accept a static default value, but I think we'd want to use a factory function that generates default values (newArrayobjects, in this case). Changing thenormalizeArrayAPI seems a bit outside the scope of this PR, so I haven't actually touched this, but have added a test for the current behavior (returningundefined) and a skipped test for the (in my mind) expected behavior (returning an empty Array).Happy to open another issue/PR to address this if it's something you'd like changed.