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

Fix Equality To Be Consistent With Compare In Version #12

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isomarcte
Copy link

This commit fixes the def equals method to be consistent with compare in Version. The reason for the inconsistency was that data-class was generating an equals method based off the repr (e.g. this.repr == that.repr), however compare is encoded to determine logical equality of a binary API with respect to SemVer (sort of). That is to say, compare compares two values by padding them to equal numeric length and then dropping the metadata value.

Unfortunately, we can not simply override the equals method and use the @data annotation (attempting to do so causes a compilation error). Thus this commit re-implements the methods from @data, but only for Version. Data types which do not implement Ordered do not need this special case treatment.

The implementation of compare likely bares some further discussion. It creates a situation where the hashCode and equality are unexpected for users cases other than comparing binary APIs.

For example, if I want to create a Set of Version values, this is likely unexpected behavior.

scala> Set(Version("1.0.0"), Version("1.0"), Version("1.0.0+SOME_META"))
val res0: scala.collection.immutable.Set[coursier.version.Version] = Set(Version(1.0.0))

Though there is no other way to implement equals and hashCode such that it is consistent with the current implementation of compare, however changing compare may likely have some unintended consequences as well.

This commit fixes the `def equals` method to be consistent with compare in `Version`. The reason for the inconsistency was that `data-class` was generating an `equals` method based off the `repr` (e.g. `this.repr == that.repr`), however `compare` is encoded to determine logical equality of a binary API with respect to SemVer (sort of). That is to say, `compare` compares two values by padding them to equal numeric length and then dropping the metadata value.

Unfortunately, we can not simply override the `equals` method _and_ use the `@data` annotation (attempting to do so causes a compilation error). Thus this commit re-implements the methods from `@data`, but _only_ for `Version`. Data types which do not implement `Ordered` do not need this special case treatment.

The implementation of `compare` likely bares some further discussion. It creates a situation where the `hashCode` and equality are unexpected for users cases other than comparing binary APIs.

For example, if I want to create a `Set` of `Version` values, this is likely unexpected behavior.

```scala
scala> Set(Version("1.0.0"), Version("1.0"), Version("1.0.0+SOME_META"))
val res0: scala.collection.immutable.Set[coursier.version.Version] = Set(Version(1.0.0))
```

Though there is no other way to implement `equals` and `hashCode` such that it is consistent with the current implementation of `compare`, however changing `compare` may likely have some unintended consequences as well.
@isomarcte
Copy link
Author

This PR is a draft until #13 is resolved.

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

Successfully merging this pull request may close these issues.

1 participant