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

fix(equals): {} and [] should not be considered equivalent #2852

Closed
wants to merge 1 commit into from

Conversation

appsforartists
Copy link
Contributor

angular.equals was returning inconsistent values for the comparison between
{} and []:

angular.equals({}, []) // true
angular.equals([], {}) // false

Since these object are not of the same type, they should not be considered
equivalent.

Fixes #2851

@appsforartists
Copy link
Contributor Author

bc72b42 is the commit I'm issuing the pull request for. I tried to make a new branch so my retina fix for #2752 wouldn't get stacked with this one, but I appear to have failed. Sorry about that.

I need to practice my git-fu.

@appsforartists
Copy link
Contributor Author

If you accept #2775 first, it should clean up the changelog. Sorry again for the crossover.

@jtymes
Copy link
Contributor

jtymes commented Jun 3, 2013

Code LGTM. This issue reminds me of this 😄

@appsforartists
Copy link
Contributor Author

Yeah, it's a pretty esoteric bug that may not have affected anyone, but it makes this test fail when I'm fixing #2621.

@vendethiel
Copy link

Code LGTM. This issue reminds me of this

Actually, this is all expected if you know how JS works. So long you know what valueOf is, of course.

@petebacondarwin
Copy link
Contributor

PR Checklist (Minor Bugfix)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name)
  • PR doesn't introduce new api
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@petebacondarwin
Copy link
Contributor

@appsforartists - can you sign the CLA please?

@appsforartists
Copy link
Contributor Author

I think I already have, but I signed it again just-in-case.

  • Brenton Simpson

On Wed, Jul 17, 2013 at 6:45 AM, Pete Bacon Darwin <notifications@github.com

wrote:

@appsforartists https://github.com/appsforartists - can you sign the
CLA please?


Reply to this email directly or view it on GitHubhttps://github.com//pull/2852#issuecomment-21113466
.

@pkozlowski-opensource
Copy link
Member

@appsforartists one more thing - would you mind squashing commits? So there is only one commit in the pull request (following commit message guidelines)?

angular.equals was returning inconsistent values for the comparison between
{} and []:

    angular.equals({}, []) // true
    angular.equals([], {}]) // false

Since these object are not of the same type, they should not be considered
equivalent.
@appsforartists
Copy link
Contributor Author

@pkozlowski-opensource I just ran rebase -i and dropped the accidental commit.

@IgorMinar
Copy link
Contributor

this landed long time ago as 1dcafd1 but the PR never got closed.

thanks!

@IgorMinar IgorMinar closed this Aug 9, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

angular.equals({}, []) == true
6 participants