-
Notifications
You must be signed in to change notification settings - Fork 947
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
Add value-based toString()s for model objects #452
Add value-based toString()s for model objects #452
Conversation
@@ -50,4 +50,7 @@ public String getEncodedPath() { | |||
public List<LatLng> decodePath() { | |||
return PolylineEncoding.decode(points); | |||
} | |||
|
|||
// Stick with the default toString; decoding the polyline to get a point count or representation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest adding a toString that includes the encoded polyline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I'm still waiting on tests for this PR, amirite? |
Yes. Sorry, been busy with the day job. :) |
All good =) |
Looks like I broke your PR. Once this is in I'll roll a release. Thanks! |
Rebased to fix conflict. Still busy with the day job. I may have time to add tests later tonight or tomorrow. |
Thanks Andrew, no rush |
Added tests. And what do you know, they caught a bug in one of my toString()s. The tests work by just running The tests don't check the content of the resulting string. Since these are human-readable strings for debugging, and don't have a fixed format or direct relation to an API, I don't think testing their content is necessary, or even appropriate. If you want, could do more thorough and isolated tests by just directly constructing various permutations of the model objects, including ones with |
Looks good to me. 👍 |
Excellent. Want to grab #463 before the release, too? |
Closes #450.
This PR adds value-based custom
toString()
methods to all themodel
objects. This should be useful for debugging, including incorporation into log messages, and interactive use inside scripting environments like Matlab and Jython.Value-based toString()s are appropriate here because the model objects are value objects; their object identity doesn't matter as much as their contents, so the default
<ClassName>@<ObjectId>
toString() output is not very useful.This PR affects user-visible behavior for existing classes, so you might want to sit on it a few days to allow other users to comment.
If this approach looks good to you, I'll add some tests.