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

fix(filterFilter): correctly handle deep expression objects #9757

Closed
wants to merge 4 commits into
base: master
from

Conversation

@gkalpak
Member

gkalpak commented Oct 23, 2014

Previously, trying to use a deep expression object (i.e. an object whose properties can be objects themselves) did not work correctly. This commit refactors filterFilter, making it simpler and adding support for filtering collections of arbitrarily deep objects.

Closes #9698


BTW, I used "non-IE8" stuff (like `Array.prototype.some/every`) for the sake of conciseness and clarity, so this is not directly back-portable to 1.2.x. If there is interest, I can create a IE8 compatible version (using `for-loops` etc).
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Oct 23, 2014

Member

Some considerations:

  1. Should we mention in the docs that deep objects can be also filtered (or is it redundant) ?
  2. Should there be inline comments at certain parts explaining that whats and the whys (or is it clear enough).
  3. None of the previous test-cases broke. Yet, since the filter was heavily refactored, one can't be really sure that all corner-cases are handled the same as before.
    (The documented stuff does seem to still work as expected, so this shouldn't introduce a breaking change (in the "formal" sense).)
Member

gkalpak commented Oct 23, 2014

Some considerations:

  1. Should we mention in the docs that deep objects can be also filtered (or is it redundant) ?
  2. Should there be inline comments at certain parts explaining that whats and the whys (or is it clear enough).
  3. None of the previous test-cases broke. Yet, since the filter was heavily refactored, one can't be really sure that all corner-cases are handled the same as before.
    (The documented stuff does seem to still work as expected, so this shouldn't introduce a breaking change (in the "formal" sense).)

@mary-poppins mary-poppins added cla: yes and removed cla: no labels Oct 23, 2014

@Narretz

This comment has been minimized.

Show comment
Hide comment
@Narretz

Narretz Oct 23, 2014

Contributor

Great work!
So the fix is for with deep expression objects with multiple properties and the feat is that these objects can be nested arbitrarily deep?

I don't want to piggyback on the PR or anything, but can you check if this PR can be incorporated in your fix? It seems like a good fit. #6623

We could also check other filter issues for possible tests to add.

Contributor

Narretz commented Oct 23, 2014

Great work!
So the fix is for with deep expression objects with multiple properties and the feat is that these objects can be nested arbitrarily deep?

I don't want to piggyback on the PR or anything, but can you check if this PR can be incorporated in your fix? It seems like a good fit. #6623

We could also check other filter issues for possible tests to add.

Show outdated Hide outdated src/ng/filter/filter.js
@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Oct 23, 2014

Contributor

So yeah, I would just ditch the first commit --- 2 lines between specs are nice, and used in most of the test suite --- removing them just makes it more inconsistent. As for the minor whitespace changes, those should be picked up by jscs once we get it doing that, but who really cares if we're honest?

Contributor

caitp commented Oct 23, 2014

So yeah, I would just ditch the first commit --- 2 lines between specs are nice, and used in most of the test suite --- removing them just makes it more inconsistent. As for the minor whitespace changes, those should be picked up by jscs once we get it doing that, but who really cares if we're honest?

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Oct 23, 2014

Member

@caitp:
I am totally OK with two lines between specs as I am with one line.
What I can't stand is inconsistency (2 lines between some specs and 1 line between others). I just removed 3 double-lines to make it consistent across the test-suite (which mostly uses 1 line between specs).

I also removed empty new lines before a closing }); in 3 cases to make it consistent across the test-suite (which also mostly doesn't have empty lines before closing });).

Anyway, those changes (along with a few white-space changes, to make things consistent and inline with the new jscs rules) are placed in a separate commit, so they can't be ignored (only...you know...they really shouldn't, because consistency is a good thing).
If it makes you feel better, I can add double-lines between all specs in the test-suite :)

Member

gkalpak commented Oct 23, 2014

@caitp:
I am totally OK with two lines between specs as I am with one line.
What I can't stand is inconsistency (2 lines between some specs and 1 line between others). I just removed 3 double-lines to make it consistent across the test-suite (which mostly uses 1 line between specs).

I also removed empty new lines before a closing }); in 3 cases to make it consistent across the test-suite (which also mostly doesn't have empty lines before closing });).

Anyway, those changes (along with a few white-space changes, to make things consistent and inline with the new jscs rules) are placed in a separate commit, so they can't be ignored (only...you know...they really shouldn't, because consistency is a good thing).
If it makes you feel better, I can add double-lines between all specs in the test-suite :)

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Oct 23, 2014

Contributor

these are test cases, consistency for minor things like that doesn't matter aw hole lot, so long as they are at least following the formal style guidelines imo. I suggest opening a separate bug for fixing style nits unrelated to this functional change so that it's easier to review (less gunk in the diff)

Contributor

caitp commented Oct 23, 2014

these are test cases, consistency for minor things like that doesn't matter aw hole lot, so long as they are at least following the formal style guidelines imo. I suggest opening a separate bug for fixing style nits unrelated to this functional change so that it's easier to review (less gunk in the diff)

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Oct 23, 2014

Member

@caitp: Hm...I thought putting those changes in a separate commit was enough, but it apparently isn't. I'll remove the commit and put it in a separate PR then.

@Narretz: I am not sure if it's a fix or a feat. It depends on how it was supposed to work. If filterFilter was supposed to handle deep expressions (and filter deep objects) then it's a fix. If it wasn't supposed to support that in the first place then it's a feat :)
Will look into other filterFilter related issues, but I can't be sure which ones you want to get in and which you don't. If you can point out PRs that should get merged, I will gladly "adapt" them to feet the refactored filterFilter.

Regarding #6623:
It sure could be incorporated, but I want to point out that:
Right now, it is possible to use {somekey: undefined} to match objects that do not have a somekey property (or have it with a value of undefined). If we merge #6623 this will be only possible using a custom function. So, we might be removing a feature :)
But, if you think it should get merged, I am totally OK with that as well.

Member

gkalpak commented Oct 23, 2014

@caitp: Hm...I thought putting those changes in a separate commit was enough, but it apparently isn't. I'll remove the commit and put it in a separate PR then.

@Narretz: I am not sure if it's a fix or a feat. It depends on how it was supposed to work. If filterFilter was supposed to handle deep expressions (and filter deep objects) then it's a fix. If it wasn't supposed to support that in the first place then it's a feat :)
Will look into other filterFilter related issues, but I can't be sure which ones you want to get in and which you don't. If you can point out PRs that should get merged, I will gladly "adapt" them to feet the refactored filterFilter.

Regarding #6623:
It sure could be incorporated, but I want to point out that:
Right now, it is possible to use {somekey: undefined} to match objects that do not have a somekey property (or have it with a value of undefined). If we merge #6623 this will be only possible using a custom function. So, we might be removing a feature :)
But, if you think it should get merged, I am totally OK with that as well.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Oct 24, 2014

Contributor

@gkalpak you probably need to rebase now that the other part is merged. yeah you do

Contributor

caitp commented Oct 24, 2014

@gkalpak you probably need to rebase now that the other part is merged. yeah you do

@Narretz

This comment has been minimized.

Show comment
Hide comment
@Narretz

Narretz Oct 24, 2014

Contributor

@gkalpak Regarding #6623; didn't think about that, good point. Leave it as is. :)

Contributor

Narretz commented Oct 24, 2014

@gkalpak Regarding #6623; didn't think about that, good point. Leave it as is. :)

@@ -129,6 +141,7 @@ describe('Filter: filter', function() {
expect(filter(items, '!isk')[0]).toEqual(items[1]);
});

This comment has been minimized.

@gkalpak

gkalpak Oct 25, 2014

Member

Unrelated newline change (but I promise it is just this one @caitp 😃).

@gkalpak

gkalpak Oct 25, 2014

Member

Unrelated newline change (but I promise it is just this one @caitp 😃).

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Oct 25, 2014

Member

@caitp: Rebased !

I am in the process of reviewing other open issues related to filterFilter (as @Narretz), in order to find tests or test-cases that might fit in or issues that would be solved once this lands or PRs made obsolete by this one etc.
I will post my findings soon (yeah, hold your breath 😛).

Member

gkalpak commented Oct 25, 2014

@caitp: Rebased !

I am in the process of reviewing other open issues related to filterFilter (as @Narretz), in order to find tests or test-cases that might fit in or issues that would be solved once this lands or PRs made obsolete by this one etc.
I will post my findings soon (yeah, hold your breath 😛).

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Oct 25, 2014

Member

Here it is: #9788
I didn't find anything that could be ported into this commit. Most of the issues should be closed anyway (imo).

There is still the feature request for supporting the filtering of objects (in addition to arrays).
There are PRs already submitted for this feature (which should be updated if this one lands of course). In any case, I believe we should not try to put more features into this one.

So, as far as I am concerned, this should be good to review/land. WDYT ?

Member

gkalpak commented Oct 25, 2014

Here it is: #9788
I didn't find anything that could be ported into this commit. Most of the issues should be closed anyway (imo).

There is still the feature request for supporting the filtering of objects (in addition to arrays).
There are PRs already submitted for this feature (which should be updated if this one lands of course). In any case, I believe we should not try to put more features into this one.

So, as far as I am concerned, this should be good to review/land. WDYT ?

@albertboada

This comment has been minimized.

Show comment
Hide comment
@albertboada

albertboada Oct 27, 2014

Amazing refactor. Make it land ASAP, please. Filtering with deep expressions has been broken for too much time now!!

albertboada commented Oct 27, 2014

Amazing refactor. Make it land ASAP, please. Filtering with deep expressions has been broken for too much time now!!

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Oct 27, 2014

Contributor

We have a fan of this fix =)

Contributor

caitp commented Oct 27, 2014

We have a fan of this fix =)

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Oct 27, 2014

Contributor

Anyways, I admit I haven't done a thorough review of this (I've tried, but it's hard to get through the whole thing in unified diffs, and text is just unreadable in the split view). I think it's probably good since tests are green, but we really don't want any surprises

Contributor

caitp commented Oct 27, 2014

Anyways, I admit I haven't done a thorough review of this (I've tried, but it's hard to get through the whole thing in unified diffs, and text is just unreadable in the split view). I think it's probably good since tests are green, but we really don't want any surprises

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Nov 10, 2014

Member

Christmas came early this year:

  • Neat fix/feat for filter filter (properly supporting deep expression objects).
  • + Tests.
  • + 9 closable issues/PRs (see #9788) - 10 including this one.
  • + Improved code readability/maintanability.
  • + ~20% LOC reduction for filter filter.

All for a tiny review (80 LOC) !
The first reviewer to give a LGTM wins a free review !!!*

🎅 Ho ! Ho ! Ho ! 🎅

_\_*: max. 100 LOC, core AngularJS-related code, must be claimed by the end of 2014
Member

gkalpak commented Nov 10, 2014

Christmas came early this year:

  • Neat fix/feat for filter filter (properly supporting deep expression objects).
  • + Tests.
  • + 9 closable issues/PRs (see #9788) - 10 including this one.
  • + Improved code readability/maintanability.
  • + ~20% LOC reduction for filter filter.

All for a tiny review (80 LOC) !
The first reviewer to give a LGTM wins a free review !!!*

🎅 Ho ! Ho ! Ho ! 🎅

_\_*: max. 100 LOC, core AngularJS-related code, must be claimed by the end of 2014

@lgalfaso lgalfaso removed this from the 1.3.4 milestone Nov 25, 2014

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Nov 27, 2014

fix(filterFilter): don't match primitive sub-expressions against any …
…prop

Basically, implement the logic detailed in the 2nd point of
angular#9757 (comment)

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Dec 1, 2014

fix(filterFilter): don't match primitive sub-expressions against any …
…prop

Basically, implement the logic detailed in the 2nd point of
angular#9757 (comment)

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Dec 1, 2014

fix(filterFilter): don't match primitive sub-expressions against any …
…prop

Basically, implement the logic detailed in the 2nd point of
angular#9757 (comment)

@pkozlowski-opensource pkozlowski-opensource modified the milestones: 1.3.5, 1.3.6 Dec 1, 2014

gkalpak added some commits Oct 23, 2014

fix(filterFilter): correctly handle deep expression objects
Previously, trying to use a deep expression object (i.e. an object whose
properties can be objects themselves) did not work correctly.
This commit refactors `filterFilter`, making it simpler and adding support
for filtering collections of arbitrarily deep objects.

Closes #7323
Closes #9698
fix(filterFilter): don't match primitive sub-expressions against any …
…prop

Basically, implement the logic detailed in the 2nd point of
#9757 (comment)
@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 2, 2014

Contributor

So I know I threatened to land this a few weeks ago --- I think we want this, but it looks like some stuff has been added since, and it's really hard to keep track of this diff u_u

Generally I trust @gkalpak's judgement though, so it's probably good. @petebacondarwin can we land this today?

Contributor

caitp commented Dec 2, 2014

So I know I threatened to land this a few weeks ago --- I think we want this, but it looks like some stuff has been added since, and it's really hard to keep track of this diff u_u

Generally I trust @gkalpak's judgement though, so it's probably good. @petebacondarwin can we land this today?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 2, 2014

Member

It's a big diff but I like that there are only additions in the test file and this is not the most central part of Angular. Let's get this in.

Member

petebacondarwin commented Dec 2, 2014

It's a big diff but I like that there are only additions in the test file and this is not the most central part of Angular. Let's get this in.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 2, 2014

Contributor

Alright *fingers crossed no regressions*

Contributor

caitp commented Dec 2, 2014

Alright *fingers crossed no regressions*

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 2, 2014

Contributor

IT IS DONE

Contributor

caitp commented Dec 2, 2014

IT IS DONE

@gkalpak gkalpak closed this in f7cf846 Dec 2, 2014

gkalpak added a commit that referenced this pull request Dec 2, 2014

fix(filterFilter): don't match primitive sub-expressions against any …
…prop

Basically, implement the logic detailed in the 2nd point of
#9757 (comment)
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 2, 2014

Member

OMG

I think I'm gonna 😂

Member

gkalpak commented Dec 2, 2014

OMG

I think I'm gonna 😂

@gkalpak gkalpak deleted the gkalpak:filterFilter-deep-expr-obj branch Dec 7, 2014

shuhei pushed a commit to shuhei/angular.js that referenced this pull request Dec 10, 2014

Shuhei Kagawa
fix(filterFilter): filter deep object by string
Enable filterFilter to filter deep object by string again. It
used to work like this before #9757, 1.3.6.

shuhei pushed a commit to shuhei/angular.js that referenced this pull request Dec 10, 2014

Shuhei Kagawa
fix(filterFilter): filter deep object by string
Enable filterFilter with string expression to filter objects with deep
properties. It used to work like this before #9757 and v1.3.6.

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Dec 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment