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

Add failing spec for sorted block with missing sorted property #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Stuk
Copy link
Contributor

@Stuk Stuk commented Sep 3, 2013

The number of items in the array increase(!) when the property that is sorted on does not exist in the objects, and is then added.

@Stuk
Copy link
Contributor Author

Stuk commented Sep 3, 2013

Added another failing spec :(

@kriskowal
Copy link
Member

Joy. Nothing can be simple :P

@kriskowal
Copy link
Member

I’m now convinced that this should be solved by omitting values from the output if the relation produces null. This probably involves tracking whether each value is currently included in the output.

SortedArray in Collections should be modified to complain loudly if someone adds an incomparable value, as SortedSet already does.

@Stuk
Copy link
Contributor Author

Stuk commented Oct 18, 2013

relation produces null

I'm not sure what you mean by this. Would it mean that line 43 of the diff would read

expect(object.sorted.length).toEqual(0);

but the remainder of that test would remain the same?

@kriskowal
Copy link
Member

@Stuk Yes, I believe that might work.

@hthetiot
Copy link
Contributor

hthetiot commented Dec 28, 2017

Need montagejs/collections#171

@hthetiot hthetiot self-requested a review December 28, 2017 11:55
@hthetiot
Copy link
Contributor

montagejs/collections#171 landed in collections master need release.

@hthetiot hthetiot added this to the 4.0.x milestone Dec 28, 2017
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

Successfully merging this pull request may close these issues.

3 participants