Skip to content
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

Feature.Equals ignores id and properties #80

Open
supermihi opened this issue Jun 4, 2017 · 7 comments
Open

Feature.Equals ignores id and properties #80

supermihi opened this issue Jun 4, 2017 · 7 comments

Comments

@supermihi
Copy link
Contributor

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?

@matt-lethargic
Copy link
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

@matt-lethargic
Copy link
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?

@supermihi
Copy link
Contributor Author

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.

@matt-lethargic
Copy link
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.

@supermihi
Copy link
Contributor Author

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.

@MattFenner
Copy link

May want to look at #115 at the same time

@xfischer
Copy link
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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants