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

Implement equals in ColumnTypes. #860

Merged
merged 3 commits into from
Apr 14, 2020
Merged

Implement equals in ColumnTypes. #860

merged 3 commits into from
Apr 14, 2020

Conversation

AliLozano
Copy link
Contributor

In Column the equals is:

   override fun equals(other: Any?): Boolean {
        if (this === other) return true
        if (other !is Column<*>) return false
        if (!super.equals(other)) return false

        if (table != other.table) return false
        if (name != other.name) return false
        if (columnType != other.columnType) return false

        return true
    `}`

It compares column type, but column types are only compared by reference because they don't implement equals and always return false inclusive when they are the same.

@Tapac
Copy link
Contributor

Tapac commented Apr 7, 2020

@AliLozano, thank you for a PR, but looks like you forgot to check nullable field of ColumnType

@AliLozano
Copy link
Contributor Author

You're right @Tapac thanks, I've added it.

@Tapac Tapac merged commit 319178b into JetBrains:master Apr 14, 2020
@spand
Copy link
Contributor

spand commented Apr 24, 2020

hashcode seems to be missing.

Joshua Bloch says on Effective Java

You must override hashCode() in every class that overrides equals(). Failure to do so will result in a violation of the general contract for Object.hashCode(), which will prevent your class from functioning properly in conjunction with all hash-based collections, including HashMap, HashSet, and Hashtable.

@Tapac
Copy link
Contributor

Tapac commented Apr 27, 2020

@spand , nice point. I'll fix it master. It's easy to forget such esential things when you live in world of data classes for too long :D

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.

3 participants