Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix (filterFilter): #6098

Closed
wants to merge 2 commits into from
Closed

fix (filterFilter): #6098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da7a2ee
fix(filterFilter): allow both dotted predicate object fields, and nes…
amj Feb 3, 2014
26c2b28
reorder tests
amj Feb 3, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ng/filter/filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ function filterFilter() {
(function(path) {
if (typeof expression[path] == 'undefined') return;
predicates.push(function(value) {
return search(path == '$' ? value : (value && value[path]), expression[path]);
return search(path == '$' ? value : ( (value && value[path]) || getter(value, path)), expression[path]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this behavior could lead to some pretty interesting collisions.

I think it would be better to use different syntax to distinguish between these two behaviors.

We thought about prefixing path with $ since that is already being used, but it just looks weird:

items | filter: {'$some.deep.path': value}

maybe something like this would be easier to understand:

items | filter: {'path: some.deep.path': value}

but even that looks weird.

another option which is the one that I'm currently preferring is:

items | filter: {some: {deep: {path: value}}}

it's a bit verbose but very explicit - there are no surprises here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the final option is very clear!

Can you elaborate on the collisions? I think the resolution order here is clear:

  1. If path is a dotted property that exists on value, use that.
  2. Otherwise, it traverses an object as expected.
    Can you give an example of the collisions you're expecting?

Considering that dotted property names are usually linter errors, and generally are artifacts of a serializer, i think you can make the case that nested objects are a much more natural usecase than supporting dotted properties. Whatever that means for an intuitive syntax, though, I don't know :) The last one looks a bit like MongoDB's query language, so maybe it's less surprising than I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

{
  'some.path': val1,
  some: { path: val2
}

object like this could be pretty confusing to work with if we simply used the fallback method.

I agree that it's not common, but I'd rather avoid it altogether.

});
})(key);
}
Expand Down
15 changes: 12 additions & 3 deletions test/ng/filter/filterSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('Filter: filter', function() {
expect(filter(items, function(i) {return i.done;}).length).toBe(1);
});

it('should take object as perdicate', function() {
it('should take object as predicate', function() {
var items = [{first: 'misko', last: 'hevery'},
{first: 'adam', last: 'abrons'}];

Expand All @@ -61,13 +61,22 @@ describe('Filter: filter', function() {
});


it('should support predicat object with dots in the name', function() {
it('should support predicate object with dots in the name', function() {
var items = [{'first.name': 'misko', 'last.name': 'hevery'},
{'first.name': 'adam', 'last.name': 'abrons'}];

expect(filter(items, {'first.name':'', 'last.name':''}).length).toBe(2);
expect(filter(items, {'first.name':'misko', 'last.name':''})).toEqual([items[0]]);
});
});


it('should also support nested objects in predicate', function() {
var items = [{'person': 'misko hevery', 'job': {name: 'bit jockey'}},
{'person': 'adam abrons', 'job': {name: 'pixelmancer'}}];

expect(filter(items, {'job.name':''}).length).toBe(2);
expect(filter(items, {'job.name':'bit'})).toEqual([items[0]]);
});


it('should match any properties for given "$" property', function() {
Expand Down