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

Route options should be serialized/deserialized using backend names #895

Merged
merged 1 commit into from
Sep 27, 2018

Conversation

osana
Copy link
Contributor

@osana osana commented Sep 26, 2018

closes #894

RouteOptions.test_toJson test is still failing com.google.gson.stream.MalformedJsonException

@Guardiola31337 There must be some typo... that I cannot see. Could you 👀 .

.requestUuid("uuid1")
.build();

String jsonString = routeOptions.toString();
Copy link
Contributor

@Guardiola31337 Guardiola31337 Sep 27, 2018

Choose a reason for hiding this comment

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

@osana
Found it 🎉

Should be routeOptions.toJson() instead of toString().

In any case, we should be careful because we can't compare json strings as we're doing here (compareJson below), because the order in which the json fields are represented is taken into account in the assertion which doesn't mean that the JSON is incorrect.

I've also noticed a discrepancy in the coordinates which we should fix 👀

Expected

"coordinates":[{"type":"Point","coordinates":[-3.707788,40.395039]},{"type":"Point","coordinates":[-3.712179,40.401819]}]

Actual

"coordinates":[[-3.707788,40.395039],[-3.712179,40.401819]]

@osana
Copy link
Contributor Author

osana commented Sep 27, 2018

@Guardiola31337 thank you for your 👀 .

Please have a look. As per our conversation in slack - I think we should stick with

"coordinates":[[-3.707788,40.395039],[-3.712179,40.401819]] format.
and use PointSerializer / PointDeserializer to do that.

Right now PointDeserializer is able to handle :
"coordinates":[{"type":"Point","coordinates":[-3.707788,40.395039]},{"type":"Point","coordinates":[-3.712179,40.401819]}]
Which is actually incorrect. #891 will be addressing this.

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Some minor nit picks.

@@ -214,4 +215,94 @@ public void toJson_fromJson() throws Exception {

assertEquals(routeOptions, routeOptionsFromJson);
}

@Test
public void test_fromJson() {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT test from test_fromJson is redundant IMO


@Test
public void test_toJson() {

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT unnecessary empty line

"\"banner_instructions\": true," +
"\"voice_units\": \"imperial\"," +
"\"uuid\": \"uuid1\"}";

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT unnecessary empty line

using the matching names from backend api
@osana
Copy link
Contributor Author

osana commented Sep 27, 2018

@Guardiola31337 I believe all NITs are fixe now.

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments @osana :shipit:

@osana osana merged commit 402d63e into master Sep 27, 2018
@osana osana deleted the osana-894 branch September 27, 2018 18:12
@osana osana mentioned this pull request Nov 12, 2018
13 tasks
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.

RouteOptions#fromJson is not working correctly
2 participants