fix(filterFilter): make $ match properties on deeper levels as well
#10408
Conversation
|
CLAs look good, thanks! |
| @@ -90,7 +90,8 @@ describe('Filter: filter', function() { | |||
| var items = [{person: {name: 'John'}}, | |||
| {person: {name: 'Rita'}}, | |||
| {person: {name: 'Billy'}}, | |||
| {person: {name: 'Joan'}}]; | |||
| {person: {name: 'Joan'}}, | |||
| {person: {name: {first: 'John', last: 'Doe'}}}]; | |||
There was a problem hiding this comment.
what is the extra line adding?
There was a problem hiding this comment.
It makes sure that a string property only matches its level (as long as the key isn't $).
I.e. it tests that {name: 'Jo'} won't match against {name: {first: 'John'...}}.
See #10401 (comment); the previous test-suite was passing even with an incorrect implementation.
There was a problem hiding this comment.
fair enough --- might want to add a comment explaining this
There was a problem hiding this comment.
Sure. Do you think it would be better to put it in a separate it block ?
There was a problem hiding this comment.
I don't have a strong opinion on that --- you get some extra lines of code if you add a new spec, fine with me either way
gkalpak
force-pushed
the
filterFilter-make-$-match-deeply
branch
from
7d3b745 to
4a08457
Compare
|
@caitp: updated |
| it('should match properties on the same level for non-$ keys', function() { | ||
| var items = [{person: 'John'}, | ||
| {person: {name: 'John'}}, | ||
| {person: {name: {first: 'John', last: 'Doe'}}}]; |
There was a problem hiding this comment.
i still want a comment clearly explaining this line, but otherwise lgtm
There was a problem hiding this comment.
I put it in a new it block (along with 2 new lines :)). Isn't the it description clear enough ?
(BTW, take a look at #10401 (comment). You might not even want this behaviour...)
There was a problem hiding this comment.
No this is the right behaviour --- named properties only match against that named property in that level. I thought you meant adding another it block in addition to the one that was already there.
I think the spec name isn't really clear about the intention of that line, it takes some digging to get the intent of it.
There was a problem hiding this comment.
I did add another it in addition to the one that was already there. The one already there is at https://github.com/angular/angular.js/pull/10408/files#diff-32d73862cc8a915aba9ab0d7b1c78c17R89.
The git diff shows it as if I replaced should respect the depth level of a "$" property with this one, but they are unrelated. (It's actually the following test that replaces the old one.)
How about that:
should match named properties only against named properties on the same level
gkalpak
force-pushed
the
filterFilter-make-$-match-deeply
branch
from
4a08457 to
15e2c95
Compare
|
@caitp: Renamed the test-case, added comments next to each line (hopefully the intent is clear now). Regarding the breaking change (not allowing named properties to match properties on deeper levels), it was introduced in the original PR (#9757), so it wouldn't make much sense to add it in this PR's commit message, right ? |
|
Sure, that works too |
|
LGTM |
|
I am somehow on the fence on this as the code is getting more complex, but given that this is isolated and it fixes a regression makes things a lot better. |
|
landed as bd28c74 |
Closes #10401