Skip to content

Conversation

@sdjacobs
Copy link
Contributor

This PR adds a DurationComparator so that returned itineraries can be sorted by duration, instead of the default method.

More Comparator implementations can be added following this pattern.

…ntations can be added following this pattern.
@gmellemstrand
Copy link
Contributor

Looks good! Maybe PathComparator should be called something else, like ArrivalTimeComparator? I think @t2gran suggested combining this with a pareto-set filter, but that could be added later.


/** Which path comparator to use */
public String pathComparator = null;

Copy link
Member

Choose a reason for hiding this comment

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

I cannot any places where pathComparator is set. I think a comment on how this is used(set to somethingother null) should be documented. Is the plan to integrate with some kind of configuration later, routing request parameter, or just fork the code and inject a different strategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not set explicitly in the code, but if you include configuration in router-config.json, it will get set automatically, e.g.

// router-config.json
{
  "routingDefaults": { 
     "pathComparator":"duration" 
   }
}

This happens via the existing config-setting code:

this.defaultRoutingRequest = scraper.scrape(routingDefaultsNode);

I'll add code to allow this to be set via an API parameter as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the clarification. I think your solution is the best, it might be confusing to have "config" params only.

@sdjacobs sdjacobs requested a review from a team as a code owner July 24, 2018 13:35
@t2gran t2gran merged commit 0967dfd into opentripplanner:master Oct 18, 2018
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