feat(filterFilter): compare object with custom `toString()` to primitive #10548

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@m7r
Contributor
m7r commented Dec 22, 2014

Closes #10464

@googlebot googlebot added the cla: yes label Dec 22, 2014
@gkalpak gkalpak commented on an outdated diff Dec 22, 2014
test/ng/filter/filterSpec.js
@@ -375,6 +375,29 @@ describe('Filter: filter', function() {
});
+ it('should consider custom `toString()` in non-strict comparison', function() {
+ var obj = new Date(0);
+ var items = [{test: obj}];
+ expect(filter(items, '1970').length).toBe(1);
+ expect(filter(items, 1970).length).toBe(1);
@gkalpak
gkalpak Dec 22, 2014 Member

Hm...these tests failed on some browsers (Chrome 39 on OS X). The failures are probably related to locale.
Changing new Date(0) to new Date(1970, 0) should fix the issue.

Alternatively, you could change 0 to 86400000 to ensure or timezones are in 1970, but I prefer the former approach.

@gkalpak
Member
gkalpak commented Dec 22, 2014

There is a test failing under certain browser-OS combination, which seems to be locale-specific.
Other than that, it LGTM.

@m7r
Contributor
m7r commented Dec 22, 2014

How do update the pull request or should I make a new one?

@gkalpak
Member
gkalpak commented Dec 22, 2014

@m7r: You can amend the commit and force-push to your branch; the PR will get updated automatically.
(BTW, it is better to make changes on a separate branch (not master). Take also a look here (if you haven't already).)

@m7r m7r feat(filterFilter): compare object with custom `toString()` to primitive
Closes #10464
c157a25
@m7r
Contributor
m7r commented Dec 23, 2014

@gkalpak travis don't like me. What to do now?

@gkalpak
Member
gkalpak commented Dec 23, 2014

@m7r: It was a flake. I restarted the job and it's all green now :)

@pkozlowski-opensource

Do we need both expects here? I mean, if expect(filter(items, 'foo').length).toBe(0); is true it means that it didn't throw, right?

Owner
m7r replied Jan 12, 2015

You are right.

@pkozlowski-opensource

I would split this into 2 separate tests if we really want to assume that built-in toString functions behave differently from the hand-crafted ones:

  • "should consider built-in toString() in non-strict comparison"
  • "should consider custom toString() in non-strict comparison"

Personally I would find it easier to read that way

Owner
m7r replied Jan 12, 2015

Only object.prototype.toString() is handled differently (ignored).
So built-in is maybe the wrong term too.

Owner
m7r replied Jan 12, 2015

Maybe "should consider toString() except Object.prototype.toString() in non-strict comparation"

Owner
m7r replied Jan 12, 2015

Maybe "should consider toString() except Object.prototype.toString() in non-strict comparation"

@pkozlowski-opensource pkozlowski-opensource self-assigned this Jan 9, 2015
@pkozlowski-opensource pkozlowski-opensource added this to the 1.3.x milestone Jan 9, 2015
@pkozlowski-opensource

@m7r I've left some additional comments to the tests - not a big deal, I can change those while merging, but would like to have your feedback on those.

@pkozlowski-opensource pkozlowski-opensource modified the milestone: 1.4.x, 1.3.x Jan 9, 2015
@m7r
Contributor
m7r commented Jan 12, 2015

@pkozlowski-opensource why is the milestone set from 1.3.9 to 1.4?

@m7r
Contributor
m7r commented Feb 24, 2015

@pkozlowski-opensource what are the next steps?

@petebacondarwin
Member

I tweaked this and landed it. Thanks @m7r

@hansmaad hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015
@m7r m7r + hansmaad feat(filterFilter): compare object with custom `toString()` to primitive
Closes #10464
Closes #10548
cf6b276
@netman92 netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@m7r @netman92 m7r + netman92 feat(filterFilter): compare object with custom `toString()` to primitive
Closes #10464
Closes #10548
b7a922a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment