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

Lets the user specify us dropping hints in response, resolves #1789. #3456

Merged
merged 1 commit into from
Dec 19, 2016

Conversation

daniel-j-h
Copy link
Member

@daniel-j-h daniel-j-h commented Dec 15, 2016

For #1789.

Adds an ?generate_hints=false option which lets us skip generating and
emitting hints for Waypoints. This can be used to decrease the response
size when the user does not need hints anyway.

We should think about making false the default in v6, and let advanced
users be explicit about the hint feature.

Tasklist

  • figure out a good name for this option, then document in http.md api spec
  • review
  • adjust for comments

cc @TheMarex

@TheMarex
Copy link
Member

figure out a good name for this option, then document in http.md api spec

generate_hints maybe?

We should think about making false the default in v6, and let advanced
users be explicit about the hint feature.

👍 Can you ticket this and put it in the v6 API changes ticket?

@emiltin
Copy link
Contributor

emiltin commented Dec 15, 2016

what about simply hints=false?

@daniel-j-h
Copy link
Member Author

hints is already taken - it's the place where users can send hints previously received.

Check the shiny new docs for the hints parameter:
http://project-osrm.org/docs/v5.5.0/api/#general-options

bearings_rule =
qi::lit("bearings=") >
(-(qi::short_ > ',' > qi::short_))[ph::bind(add_bearing, qi::_r1, qi::_1)] % ';';

base_rule = radiuses_rule(qi::_r1) | hints_rule(qi::_r1) | bearings_rule(qi::_r1);
base_rule = radiuses_rule(qi::_r1) //
Copy link
Contributor

Choose a reason for hiding this comment

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

I have recently found // clang-format off // clang-format on and used here https://github.com/Project-OSRM/osrm-backend/blob/opening_hours/include/util/opening_hours.hpp#L302

Would it be ok to allow suppress clang format for qi rules and use qi-usual formating like

    emit_hints_rule
        = qi::lit("emit_hints=")
        > qi::bool_[ph::bind(&engine::api::BaseParameters::emit_hints, qi::_r1) = qi::_1]
        ;

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah makes sense probably not only for Spirit grammars but e.g. also for program option parser and other similar code. Want to ticket this or make a separate pull request for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a separate ticket with low priority will be good

@daniel-j-h
Copy link
Member Author

Adjusted to comments - @oxidase and @TheMarex could you have a final look again.

@TheMarex
Copy link
Member

Flagging here: This will need an adaption in node-osrm.

Copy link
Member

@TheMarex TheMarex 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!

@TheMarex
Copy link
Member

There is one test still failing https://travis-ci.org/Project-OSRM/osrm-backend/jobs/184548957#L5796

…1789.

Adds an `generate_hints=false` option which lets us skip generating and
emitting hints for Waypoints. This can be used to decrease the response
size when the user does not need hints anyway.

We should think about making `false` the default here in v6.
@daniel-j-h
Copy link
Member Author

Ah sorry, missed updating the expected parsing failure for the emit_hints -> generate_hints name change. Updating and then merging this.

@daniel-j-h daniel-j-h merged commit b1f6797 into master Dec 19, 2016
@MoKob MoKob deleted the nohint-feature branch December 23, 2016 11:17
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.

4 participants