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

Add value-based toString()s for model objects #452

Merged
merged 1 commit into from
Jul 5, 2018
Merged

Add value-based toString()s for model objects #452

merged 1 commit into from
Jul 5, 2018

Conversation

apjanke
Copy link
Contributor

@apjanke apjanke commented Jul 2, 2018

Closes #450.

This PR adds value-based custom toString() methods to all the model 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.

@@ -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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@domesticmouse
Copy link
Contributor

I'm still waiting on tests for this PR, amirite?

@apjanke
Copy link
Contributor Author

apjanke commented Jul 4, 2018

Yes. Sorry, been busy with the day job. :)

@domesticmouse
Copy link
Contributor

All good =)

@domesticmouse
Copy link
Contributor

Looks like I broke your PR. Once this is in I'll roll a release. Thanks!

@apjanke
Copy link
Contributor Author

apjanke commented Jul 5, 2018

Rebased to fix conflict.

Still busy with the day job. I may have time to add tests later tonight or tomorrow.

@domesticmouse
Copy link
Contributor

Thanks Andrew, no rush

@apjanke
Copy link
Contributor Author

apjanke commented Jul 5, 2018

Added tests. And what do you know, they caught a bug in one of my toString()s.

The tests work by just running toString() on the "live" data used in the existing query tests.

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 null in all the nullable fields, and do toString() on those. Seems a little overkill to me.

@domesticmouse
Copy link
Contributor

Looks good to me. 👍

@domesticmouse domesticmouse merged commit c9caa61 into googlemaps:master Jul 5, 2018
@apjanke apjanke deleted the model-value-based-toStrings branch July 5, 2018 16:22
@apjanke
Copy link
Contributor Author

apjanke commented Jul 5, 2018

Excellent.

Want to grab #463 before the release, too?

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.

2 participants