Skip to content

Feature.Equals ignores id and properties #80

Open

Description

@supermihi

The equality comparison methods of the Feature class ignore the Id and Properties properties, resulting in e.g. two features with different Id being equal if the geometries are the same.

Since there is a unit test explicitly covering this behavior, I assume it is by design, but I don't understand the rationale behind; for me, it seems very counter-intuitiive and potential source of hard to trackdown bugs. Can somebody give me a clue?

Activity

  1. matt-lethargic commented on Jun 4, 2017

    @matt-lethargic
    Member

    I'll have a closer look shortly, this is code from my predecessor so I'm not 100% sure on the rationale of everything as yet

  2. matt-lethargic commented on Jun 15, 2017

    @matt-lethargic
    Member

    I've had a look at the code for this and I've refered back to the RFc 7946, but I'm still no wiser. Maybe @jbattermann has more insight into this?

  3. supermihi commented on Jun 17, 2017

    @supermihi
    ContributorAuthor

    The behavior is especially problematical in conjunction with #56: if one explicitly makes use of the properties of a feature, one most probably will want them to be considered by equality comparison.

    On the other hand, it would be very bad to have Feature and Feature<T> behave differently wrt. equality comparison.

  4. matt-lethargic commented on Jul 25, 2017

    @matt-lethargic
    Member

    Firstly there is no guidance in the RFC for how you should compare two features but it does suggest that different ID's are a different feature which makes sense. I would also agree that two features can share the same geographical space so comparison on geo location alone could be incorrect. If we were to change the existing classes then there would have to be a version bump to highlight the breaking change.

  5. supermihi commented on Jul 25, 2017

    @supermihi
    ContributorAuthor

    So, should we compare on both id and geometry? Or id and geometry and props? Including the properties in equality comparison might be too expensive for e.g. usage as dictionary keys. But on the other hand one could in such a circumstance use a custom EqualityComparer, so it might be a reasonable default?

    I agree that in any case the major version number should be increased, to be consistent with semantic versioning.

  6. MattFenner commented on Nov 29, 2018

    @MattFenner

    May want to look at #115 at the same time

  7. xfischer commented on Feb 26, 2019

    @xfischer
    Member

    OK to close this and refer to #115 ?

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions