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

Added toString, hashcode, and equal methods #450

Closed
wants to merge 4 commits into from
Closed

Conversation

cammace
Copy link
Contributor

@cammace cammace commented Apr 20, 2017

This PR adds the standard toString hashcode and equal methods to model classes. Previously, if you tried comparing two matching objects, it would return false, this fixes that.

  • Test should be added
  • Javadoc needs to be added

Copy link
Member

@zugaldia zugaldia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace I like the idea of adding these methods, but as the models and API support grow, this process is manual, tedious, and error-prone.

I'd like to propose an alternative approach to automate this process. My current thinking is to use auto-value-gson to generate all the model classes. This comes with the following benefits out of the box:

This is a larger change and before we do it I'd like to hear 👍/👎 from @mapbox/android.

@cammace
Copy link
Contributor Author

cammace commented Apr 24, 2017

I'd like to propose an alternative approach to automate this process

My original hesitation for doing this was the overhead that comes with maintaining all these methods. Any means to automate this process sounds great to me.

@cammace cammace mentioned this pull request Jul 20, 2017
10 tasks
@cammace
Copy link
Contributor Author

cammace commented Jul 20, 2017

Since AutoValue will require a bunch of API breaking changes we are holding off till 3.0.0 gets released (discussed in #499). Moving forward with this PR in the meantime. Can I get your review again @zugaldia

@zugaldia
Copy link
Member

Were all tests and Javadoc added to the PR, or is this still pending as per OP?

@cammace
Copy link
Contributor Author

cammace commented Jul 27, 2017

I chose not to include test and Javadoc since we plan to switch to AutoValues very soon (which won't require us to test this).

@cammace
Copy link
Contributor Author

cammace commented Jul 31, 2017

If you agree that we don't need to add test/javadoc for this, could I get another round of reviews?

@cammace
Copy link
Contributor Author

cammace commented Aug 1, 2017

Removing from the 2.2.0 milestone since this isn't an immediate feature needed and most likely the 3.0 release will bring AutoValues which handles this for us.

@cammace cammace removed this from the v2.2.0 milestone Aug 1, 2017
@Guardiola31337
Copy link
Contributor

Noting here that when using Kotlin Data Classes the compiler automatically derives equals()/hashCode() and toString() providing everything you need to make AutoValue (in most cases) not necessary (avoiding a 3rd party dependency).

@cammace
Copy link
Contributor Author

cammace commented Sep 21, 2017

AutoValue works coming in 3.0.0 release which resolves all the things mentioned in this PR, closing.

@cammace cammace closed this Sep 21, 2017
@cammace cammace deleted the cam-440 branch September 21, 2017 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants