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

GH #122: Enable CountryRules by default #150

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rsarathy
Copy link

@rsarathy rsarathy commented May 27, 2024

No description provided.

@rsarathy rsarathy requested a review from graue May 27, 2024 17:28
Copy link

@graue graue left a comment

Choose a reason for hiding this comment

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

Why disable the Austria and Germany country rules? Are they hurting something?

Someone might want to run a BikeHopper instance there at some point, and it's an additional divergence from upstream GH.

@rsarathy
Copy link
Author

rsarathy commented Jun 3, 2024

Why disable the Austria and Germany country rules? Are they hurting something?

Someone might want to run a BikeHopper instance there at some point, and it's an additional divergence from upstream GH.

No, they don't. I thought it would make the code slimmer but you're right, we do want to stay parallel to GH as much as possible. I reverted those changes.

@rsarathy rsarathy requested a review from graue June 3, 2024 19:05
Copy link

@graue graue left a comment

Choose a reason for hiding this comment

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

Seems fine. Do you plan to pursue this part of #122 as well?

correct operation requires country rules and they should not be disabled by default, nor should the feature to disable them exist. Ideally we would merge this change into upstream (which is also currently failing to handle cycleway:left and cycleway:right at all).

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.

2 participants