Parse old annotations parameter true as new annotations_type all #3692

karenzshea · 2017-02-10T16:31:05Z

Issue

Closes #3684

Tasklist

add regression / cucumber cases (see docs/testing.md)
review
adjust for comments

Requirements / Relations

Sort of fixes Project-OSRM/node-osrm#297 but doesn't really matter, this pr mostly preserves backwards compatibility

oxidase · 2017-02-10T16:48:50Z

include/engine/api/route_api.hpp

        {
+            if (parameters.annotations == true)
+                parameters.annotations_type = RouteParameters::AnnotationsType::All;


This will break new functionality with a list of values. It will always fallback to All

oxidase · 2017-02-10T16:49:26Z

include/engine/api/route_api.hpp

        {
+            if (parameters.annotations == true)
+                parameters.annotations_type = RouteParameters::AnnotationsType::All;


This will break new functionality with a list of values. It will always fallback to All

chaupow · 2017-02-13T09:48:42Z

include/engine/api/route_api.hpp

@@ -228,42 +228,50 @@ class RouteAPI : public BaseAPI

        std::vector<util::json::Object> annotations;

-        if (parameters.annotations_type != RouteParameters::AnnotationsType::None)
+        if (parameters.annotations_type != RouteParameters::AnnotationsType::None ||
+            parameters.annotations == true)


could this also be
if (parameters.annotations == true)?

I think there are technically cases where only checking for parameters.annotations could be wrong. Say with a constructor where annotations defaults to false and then annotations_type is manually set.

Ah I get your point. Like, it should be good for how we use it now but better safe than sorry?

Seems reasonable to me to keep both checks, especially since this PR is supposed to be fixing the usage case in node-osrm that I didn't think of.

ok, just noticed: definitely keep this, it'll make node-osrm prettier :D

chaupow · 2017-02-13T09:52:45Z

include/engine/api/route_api.hpp

        {
+            auto ReqAnnotations = parameters.annotations_type;
+            if ((parameters.annotations == true) & (parameters.annotations_type &
+                RouteParameters::AnnotationsType::None))


i think it should be && instead of & in the first case because it is a boolean comparison?

if ((parameters.annotations == true) && (parameters.annotations_type & RouteParameters::AnnotationsType::None))

Also I'm not sure what (parameters.annotations_type & RouteParameters::AnnotationsType::None) is doing. Is
(parameters.annotations_type == RouteParameters::AnnotationsType::None) not more intuitive and does the same? Not sure though cause I don't know much about bit-magic

(parameters.annotations_type & RouteParameters::AnnotationsType::None will always be false, needs to be == instead of &.

chaupow · 2017-02-13T09:56:52Z

include/engine/api/route_api.hpp

            for (const auto idx : util::irange<std::size_t>(0UL, leg_geometries.size()))
            {
                auto &leg_geometry = leg_geometries[idx];
                util::json::Object annotation;

-                if (parameters.annotations_type & RouteParameters::AnnotationsType::Duration)
+                if (ReqAnnotations & RouteParameters::AnnotationsType::Duration)


maybe we should add a comment explaining to the future that this basically means
"if Duration bit was set"

chaupow · 2017-02-13T09:59:06Z

looks good overall 👍
Have some questions about the first if-conditions but otherwise good to go I think.

Also:

needs to be clangformatted to make travis happy.

TheMarex

There is a logic bug and some smaller style issues.

TheMarex · 2017-02-13T10:04:33Z

include/engine/api/route_api.hpp

        {
+            auto ReqAnnotations = parameters.annotations_type;


local variables use lower_case names. Maybe annotations_type would be a good name here:

auto annotations_type = parameters.annotations_type;

TheMarex · 2017-02-13T10:05:37Z

include/engine/api/route_api.hpp

        {
+            auto ReqAnnotations = parameters.annotations_type;
+            if ((parameters.annotations == true) & (parameters.annotations_type &
+                RouteParameters::AnnotationsType::None))


(parameters.annotations_type & RouteParameters::AnnotationsType::None will always be false, needs to be == instead of &.

TheMarex · 2017-02-13T10:29:52Z

include/engine/api/route_api.hpp

+                (parameters.annotations_type == RouteParameters::AnnotationsType::None))
+            {
+                requested_annotations = RouteParameters::AnnotationsType::All;
+            }


If this code block is moved before line 231 you can change the first if to also only check for the ::None case. That reduces the points in the code that need to know about the work-a-round.

karenzshea · 2017-02-13T12:12:51Z

Also to note: this problem will go away when we remove redundant bool parameters in v6 #3644

karenzshea requested review from chaupow and TheMarex February 10, 2017 16:31

oxidase reviewed Feb 10, 2017

View reviewed changes

karenzshea force-pushed the annotations_type_true branch from 3b02154 to 2a26a45 Compare February 13, 2017 09:25

karenzshea added this to the 5.6.0 milestone Feb 13, 2017

chaupow reviewed Feb 13, 2017

View reviewed changes

TheMarex requested changes Feb 13, 2017

View reviewed changes

TheMarex reviewed Feb 13, 2017

View reviewed changes

TheMarex approved these changes Feb 13, 2017

View reviewed changes

parse parameters.annotations into AnnotationsType::All

9b12319

karenzshea force-pushed the annotations_type_true branch 2 times, most recently from 84cb198 to 6301e34 Compare February 13, 2017 17:33

add unit test for annotations=true returning all annotations

6301e34

TheMarex merged commit e75278f into master Feb 13, 2017

TheMarex deleted the annotations_type_true branch February 13, 2017 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse old annotations parameter true as new annotations_type all #3692

Parse old annotations parameter true as new annotations_type all #3692

karenzshea commented Feb 10, 2017 •

edited by TheMarex

Loading

oxidase Feb 10, 2017

oxidase Feb 10, 2017

chaupow Feb 13, 2017

karenzshea Feb 13, 2017

chaupow Feb 13, 2017

karenzshea Feb 13, 2017

chaupow Feb 13, 2017

chaupow Feb 13, 2017

TheMarex Feb 13, 2017

chaupow Feb 13, 2017

chaupow commented Feb 13, 2017 •

edited by daniel-j-h

Loading

TheMarex left a comment

TheMarex Feb 13, 2017

TheMarex Feb 13, 2017

TheMarex Feb 13, 2017

karenzshea commented Feb 13, 2017

Parse old annotations parameter true as new annotations_type all #3692

Parse old annotations parameter true as new annotations_type all #3692

Conversation

karenzshea commented Feb 10, 2017 • edited by TheMarex Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaupow commented Feb 13, 2017 • edited by daniel-j-h Loading

TheMarex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenzshea commented Feb 13, 2017

karenzshea commented Feb 10, 2017 •

edited by TheMarex

Loading

chaupow commented Feb 13, 2017 •

edited by daniel-j-h

Loading