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

RouteOptions parsing fixes #1123

Merged
merged 1 commit into from
Mar 3, 2020
Merged

Conversation

korshaknn
Copy link
Contributor

closes #1122

Changed RouteOptions to store data in Strings (not in Lists). It had to be done to save semver.

So, if you pass a list of radiuses for example, it will be stored as a String.
If you want to get a list of radiuses, inner String will be converted to a returned list.

Removed any extra logic from String <-> List converting.
So, you you pass a list with null values, they won't be removed
set radiusesList(listOf(1, 2, null, null)) -> "1;2;;"
get radiusesList() from "1;2;;" -> [1, 2, null, null]

Also changed parsing logic for annotations. They have to be separated with comma, not semicolon.

@korshaknn korshaknn added the Bug label Mar 2, 2020
@korshaknn korshaknn self-assigned this Mar 2, 2020
Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

👍

we can clean this up with next semver.

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.

Looks good to me 👍

For testing this thoroughly we can run requests before and after #1118 #1121 (to get a full JSON we might need to add all the options to the request and add a breakpoint in the response to obtain the old JSON directionsResponse.routes[0].toJson()) and check if toJson / fromJson work with the new changes.

@korshaknn
Copy link
Contributor Author

@Guardiola31337 added more tests to cover all fromJson cases.
Also check how all new list methods work with toJson.

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.

Left some minor comments, other than that this looks good to me 🚀

@korshaknn korshaknn force-pushed the knn-route-options-parsing-fixes branch from 4385db9 to 9eaa0e4 Compare March 3, 2020 14:47
}

@Test
public void excludeIsValid_fromJson() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing toJson test? Couldn't find it 👇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a common toJson test to check all values

@Test
  public void routeOptions_toJson() {
    RouteOptions options = routeOptions();

    assertEquals(ROUTE_OPTIONS_JSON, options.toJson());
}

I added separate toJson tests only for values, which can be set either as a String or a list
for example bearings(String)/bearingList(List)

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a common toJson test to check all values

👍 sounds good assertEquals(ROUTE_OPTIONS_JSON, options.toJson()); we shouldn't rely on the order of keys but that's a different story. We should think about integrating https://github.com/JakeWharton/shimo/ in a future PR.

@korshaknn korshaknn force-pushed the knn-route-options-parsing-fixes branch from 9eaa0e4 to fcd0834 Compare March 3, 2020 15:15
@korshaknn korshaknn merged commit 063bdf7 into master Mar 3, 2020
@langsmith langsmith mentioned this pull request Mar 23, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RouteOptions IllegalStateException - RouteOptions backwards incompatible
3 participants