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

Exclude waypoints from routing #1362

Merged
merged 8 commits into from
Feb 18, 2022
Merged

Exclude waypoints from routing #1362

merged 8 commits into from
Feb 18, 2022

Conversation

VysotskiVadim
Copy link
Contributor

@VysotskiVadim VysotskiVadim commented Feb 15, 2022

Description

Direction API provides a new feature "exclude waypoint". To exclude a coordinate form your route add it to the list of exclude like point(lon lat). For example if you want to exclude motorways and coordinate 1.0 1.0 use following exclude param: exclude=motorway,point(1.0%201.0)

You're reviewing a second iteration of the exclude waypoints implementation. First iteration is #1361.

@VysotskiVadim VysotskiVadim changed the title Need feedback about exclude waypoint parameter Exclude waypoints from routing Feb 16, 2022
*/
@Nullable
public abstract String exclude();

/**
* Exclude certain road types from routing. The default is to not exclude anything from the
* profile selected. The following exclude flags are available for each profile:
* Exclude certain road types and points from routing. The default is to not exclude anything
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attention ⚠️
How do you think, is that a logical breaking change? I have doubts about whether I need to filter out points from that filed. I think no. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't consider this a breaking change. It just reflects the state of the server-side API itself which wasn't considered breaking since the format is a generic string.

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.

Overall is looking good @VysotskiVadim

Left some minor comments.

* @return this builder for chaining options together
*/
@NonNull
public Builder excludeObject(@Nullable Exclude exclude) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this @Nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To let users cleanup the exclude.

  @Test
  public void cleanExcludes() {
    RouteOptions routeOptions = routeOptions().toBuilder()
        .excludeObject(null)
        .build();

    assertNull(routeOptions.exclude());
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, if you don't want to include it you don't set it to RouteOptions right? Normally, fields added to a Builder are non-nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if you build new route options from existing route options as on example ☝️


public class RouteOptionsExcludeTest {
@Test
public void parseTwoPointsWithTwoCriteriaMixed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT Normally is good to structure the tests following the triple A 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=40 http://wiki.c2.com/?ArrangeActAssert

@Test
public void parseTwoPointsWithTwoCriteriaMixed() {
    // Arrange
    
    // Act
    
    // Assert
}

Note the empty lines between Arrange / Act and Act / Assert.

The same thing applies to the tests below 👇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, that AAA brings more clarity to tests. Sometimes tests are so complex that it's not easy to understand the border between setup and an act 🙂
I added empty lines to some tests from PR. But some of the tests I find too trivial and I'm not sure if highlighting of their AAA structure will bring some benefit. What do you think about AAA structure highlighting for trivial tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

In such cases, I personally prefer to add it too, for consistency.

Comment on lines 115 to 118
} else if (excludeItem.contains("(") && excludeItem.contains(")")) {
// Ignoring the unexpected type of data. Update the library to get support of new types
} else if (excludeItem.contains("(") || excludeItem.contains(")")) {
throw new IllegalArgumentException("Can't parse parameter " + excludeItem);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove these 2 cases and let the backend respond with an appropriate errors. We should only handle what we expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, now I see it's the other way around. Regardless, since the server-side API format is generic string, we cannot throw an exception on the parsing level ourselves. We probably need to remove that if-check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line is about a case when you put a comma instead of the space in point

  @Test(expected = IllegalArgumentException.class)
  public void parsePointWithCommaDelimiter() {
    String invalidPoint = "point(5.0,7.0)";
    Exclude.fromUrlQueryParameter(invalidPoint);
  }

Do you suggest to ignore everything we can't parse? Like:

  @Test
  public void parsePointWithCommaDelimiter() {
    String invalidPoint = "point(5.0,7.0)";
    Exclude exclude = Exclude.fromUrlQueryParameter(invalidPoint);
    assertNull(exclude);
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree with @LukasPaczos providing the right format should be managed by the end-user when integrating the API and check on the server side. I believe we should also remove the previous if #1362 (comment) If in the future Directions API adds new data formats for exclude, we'll accommodate YAGNI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LukasPaczos , @Guardiola31337 , updated parsing with a new concept: Exclude is just a type-safe way of working with known excludes. All the unknown properties and flags are ignored now

Copy link
Contributor

Choose a reason for hiding this comment

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

Why even ignore it and return null from our side? Why can't we pass whatever we get from the client (assuming is well formed / formatted and let the server crash if not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Guardiola31337 , the line 184 is for reading points, not for writing. You can write incorrect point like exclude("point(test)"), but you won't be able to read it as a Point because it can't be parsed

@VysotskiVadim VysotskiVadim force-pushed the vv-exlude-point-2 branch 3 times, most recently from c75e05d to 721bee5 Compare February 17, 2022 17:43
@@ -340,6 +340,7 @@ private DirectionsCriteria() {
* @since 3.0.0
*/
@Retention(RetentionPolicy.CLASS)
// Please update Exclude.VALID_EXCLUDE_CRITERIA adding new type of exclude
Copy link
Contributor Author

@VysotskiVadim VysotskiVadim Feb 17, 2022

Choose a reason for hiding this comment

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

Don't know any good solution for reusing an array between @StringDef and Exclude for validation. Please advice if you know 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

You could maybe iterate via available DirectionsCriteria fields using reflection and compare them against the array in the Exclude class. Not a blocker in my opinion.

@@ -340,6 +340,7 @@ private DirectionsCriteria() {
* @since 3.0.0
*/
@Retention(RetentionPolicy.CLASS)
// Please update Exclude.VALID_EXCLUDE_CRITERIA adding new type of exclude
Copy link
Contributor

Choose a reason for hiding this comment

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

You could maybe iterate via available DirectionsCriteria fields using reflection and compare them against the array in the Exclude class. Not a blocker in my opinion.

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.

3 participants