Skip to content

Conversation

@kmadsen
Copy link
Contributor

@kmadsen kmadsen commented Jan 7, 2020

Adding support for junction views in the directions api https://github.com/mapbox/navigation-sdks/issues/194

Example banner instructions response which includes the view

"bannerInstructions": [
  {
    "distanceAlongGeometry": 82.8,
    "primary": ...,
    "secondary": ...,
    "view": {
      "text": "",
      "components": [
        {
          "text": "",
          "type": "guidance-view",
          "imageURL": "https://api-turn-here-staging-451578336.us-east-1.elb.amazonaws.com/guidance-views/v1/z/jct/CA075101?arrow_ids=CA07510E"
        }
      ],
      "type": "off ramp",
      "modifier": "right"
    }
  }
]
},

@langsmith
Copy link

Replace REPLACE_VERSION_KYLE with the version (i think it's 4.10)

This will be a part of a semver major release, so it'll be 5.0.0

@kmadsen kmadsen force-pushed the kyle/directions-junction-view-api branch from f379c5f to 761bf4c Compare January 8, 2020 22:29
@sonatype-lift
Copy link

sonatype-lift bot commented Jan 8, 2020

Muse analysis has completed. New issues were detected with line numbers outside of the pull request change set:

  • (AutoValueImmutableFields)[https://github.com/mapbox/mapbox-java/blob/f379c5fe936648bdfc3ac42db4b61fa217a116dd/services-directions/src/main/java/com/mapbox/api/directions/v5/models/BannerView.java#L52]: AutoValue instances should be deeply immutable. Therefore, we recommend returning ImmutableList instead. Read more at http://goo.gl/qWo9sC

@mapbox mapbox deleted a comment from sonatype-lift bot Jan 8, 2020
@langsmith
Copy link

Awesome work and welcome to Mapbox Java! 😄

@kmadsen kmadsen merged commit 93da3e6 into master Jan 8, 2020
@langsmith langsmith deleted the kyle/directions-junction-view-api branch January 8, 2020 23:18
@langsmith langsmith mentioned this pull request Jan 8, 2020
15 tasks
String jsonString = responseFromJson1.toJson();
BannerInstructions responseFromJson2 = BannerInstructions.fromJson(jsonString);

Assert.assertEquals(responseFromJson1, responseFromJson2);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT

assertEquals(responseFromJson1, responseFromJson2);

}

@Test
public void fromtestToFromJson() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo *fromtestToFromJson


@Test
public void fromtestToFromJson() {
BannerInstructions responseFromJson1 = BannerInstructions.fromJson(BANNER_INSTRUCTION_JSON);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing

Could we improve naming here? Especially responseFromJson1 and responseFromJson2 😬

}

@Test
public void fromtestToFromJson() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing Could we rename the test so it's clearer (although not consistently applied 👀 at the "naming" convention of the rest of the tests)? We should also remove test prefix from tests names, it's implicit with the usage of @Test annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noticed these comments and agree with the nits. I'm not ignoring.

Think these were copy pasted so larger clean ups will happen during free time :)

BannerInstructions responseFromJson = BannerInstructions.fromJson(BANNER_INSTRUCTION_JSON);

BannerView bannerView = responseFromJson.view();
Assert.assertEquals(bannerView.text(), "CA01610_1_E");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above re: Assert.assertEquals vs assertEquals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants