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

make CHProfile selection pluggable #1797

Merged
merged 1 commit into from
Nov 19, 2019
Merged

make CHProfile selection pluggable #1797

merged 1 commit into from
Nov 19, 2019

Conversation

karussell
Copy link
Member

@karussell karussell commented Nov 16, 2019

minor improvement for #1790 to call Weighting.matches again. A disadvantage could be that Weighting.matches is now less strict.

continue;
}
if (!vehicle.isEmpty() && !getVehicle(p).equals(vehicle)) {
if (!p.getWeighting().matches(hintsMap)) {
Copy link
Member

Choose a reason for hiding this comment

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

If we do this we probably should use .matches() also a few lines below (lines 80+81)

Copy link
Member Author

@karussell karussell Nov 19, 2019

Choose a reason for hiding this comment

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

In lines 80+81 we compare 2 Weightings, but here we want to know "if the HintsMap matches the Weighting". So, to use the matches method we would need
getWeighting().matches(hintsMap) && getWeighting().matches(hintsMap)
and still both Weighting objects could be different. Should we do this? Or, to remove the vehicle comparison, we could use the Weighting.equals method instead of String.equals?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can get rid of Weighting#matches altogether :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, have created a separate issue as this seems to be quite involved: #1800

Will merge this here for now if no objections :) ?

@karussell karussell merged commit 781b1db into master Nov 19, 2019
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.

None yet

2 participants