Conversation
|
Thanks for the PR. If you would like this to be part of the main code, then I will do a pretty strict review and you will have to change a few things. However, we can also add this as a separate Maven module and then it will be "yours" and I will not torture you with a flood of review comments. What would you like to do? |
|
A separate maven module is a good idea so I can have "ownership" over this feature and it's not your problem. I'll refactor it to do that. That said, I'd appreciate any feedback on the code. I want to learn from this project and improve my Java code style. |
assertions/src/main/java/org/opentripplanner/assertions/ItineraryAssertions.java
Outdated
Show resolved
Hide resolved
| private static String routeShortName(Route route) { | ||
| return route == null ? null : route.getShortName(); | ||
| } | ||
|
|
||
| private static String routeLongName(Route route) { | ||
| return route == null ? null : route.getLongName(); | ||
| } |
There was a problem hiding this comment.
Instead of these static methods, I would wrap the route and add getters for the various fallback cases.
| leg.distance()); | ||
| } | ||
|
|
||
| private String describeCriteria(List<LegCriterion> criteriaSet) { |
There was a problem hiding this comment.
This should be a method on LegCriterion.
| return new ItineraryMatchResult(completeMatches, partialMatches, extraLegs, errors); | ||
| } | ||
|
|
||
| private String formatLegDescription(Leg leg) { |
There was a problem hiding this comment.
I would wrap the Leg and add this method there.
There was a problem hiding this comment.
can we just add it to the Leg in the client package?
| assertThat(result.getPartialMatches().get(0).getMatchingCriteria()).contains("route '[10]'"); | ||
| assertThat(result.getPartialMatches().get(0).getMissingCriteria()).contains("mode TRAM"); |
There was a problem hiding this comment.
| assertThat(result.getPartialMatches().get(0).getMatchingCriteria()).contains("route '[10]'"); | |
| assertThat(result.getPartialMatches().get(0).getMissingCriteria()).contains("mode TRAM"); | |
| assertThat(result.getPartialMatches().getFirst().getMatchingCriteria()).contains("route '[10]'"); | |
| assertThat(result.getPartialMatches().getFirst().getMissingCriteria()).contains("mode TRAM"); |
| public class LegMatchingState { | ||
|
|
||
| private final Leg leg; | ||
| private final List<String> matchingCriteria = new ArrayList<>(); | ||
| private final List<String> missingCriteria = new ArrayList<>(); | ||
|
|
There was a problem hiding this comment.
Instead of an mutable state accumulator I would make the LegCriterion return a list of immutable results (some type of success/failure) and then the engine is responsible for accumulating the state.
That way the two responsibilities of
- checking if the criterion matches
- accumulating the state
are separate and it's easy to write more criterion, because they don't have to know to which collection they need to add their result.
danielhep
force-pushed
the
assertion-lib
branch
from
43fde09 to
12946f2
Compare
|
Is this ready to merge or do you want to work on it some more? |
|
Give me a bit longer, I am going to implement some more improvements based on your feedback. Appreciate the comments
…On Fri, Feb 27, 2026, at 3:23 AM, Leonard Ehrenfried wrote:
*leonardehrenfried* left a comment (opentripplanner/otp-java-client#132) <#132 (comment)>
Is this ready to merge or do you want to work on it some more?
—
Reply to this email directly, view it on GitHub <#132 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE2DPXHUVO2V25H5FRQOFD4N75JTAVCNFSM6AAAAACVOUU2Z2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTSNZRGUYDGOBYGY>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Is this now ready for review? |
2 participants
With this PR I'm moving the assertion library that we are using in https://github.com/daniel-heppner-ibigroup/otp-deployment-tests to run checks against the returned itineraries from OTP to ensure we're getting reasonable results. I've tried to build a nice API that allows you to define the requirements for the itineraries in code.