-
Notifications
You must be signed in to change notification settings - Fork 172
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
Deprecated parking #1390
Deprecated parking #1390
Conversation
🍱 You can preview the tagging presets of this pull request here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove those that are about below 1k as commented inline.
We have new guidelines on this in https://github.com/openstreetmap/id-tagging-schema/blob/main/GUIDELINES.md#tag-updates-and-additions which don't have a strict number, but we want to only have entries here with relevant usage.
"replace": {"parking:both": "yes"} | ||
}, | ||
{ | ||
"old": {"parking:lane:both": "no"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
"old": {"parking:lane:both": "fire_lane"}, | ||
"replace": {"parking:both": "no", "parking:both:restriction": "no_stopping", "parking:both:restriction:reason": "fire_lane"} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can keep this (even though below 1k) because the new tagging is complex.
"replace": {"parking:both": "no", "parking:both:restriction": "no_stopping", "parking:both:restriction:reason": "fire_lane"} | ||
}, | ||
{ | ||
"old": {"parking:lane:both": "separate"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"replace": {"parking:right": "no", "parking:right:restriction": "no_stopping", "parking:right:restriction:reason": "fire_lane"} | ||
}, | ||
{ | ||
"old": {"parking:lane:right": "separate"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"replace": {"parking:right": "separate"} | ||
}, | ||
{ | ||
"old": {"parking:lane:right": "diagonal"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"replace": {"parking:right": "yes", "parking:right:orientation": "diagonal"} | ||
}, | ||
{ | ||
"old": {"parking:lane:right": "parallel"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"replace": {"parking:right": "yes", "parking:right:orientation": "parallel"} | ||
}, | ||
{ | ||
"old": {"parking:lane:right": "perpendicular"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"replace": {"parking:right": "yes", "parking:right:orientation": "perpendicular"} | ||
}, | ||
{ | ||
"old": {"parking:lane:right:parallel": "on_street"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SupaplexOSM could you have a look at the tag update definitions – you will spot issues that might be there more easily than me. Update: He messaged me with a "looks good at first glance" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to be merged.
We could argue to remove the "parking:both": "yes"
which is not the ideal migration scenario. In my migration tool https://osmberlin.github.io/osm-tag-updater/#/manual I did not use this migration path but instead told users to add the proper value. But we don't have this kind of tooling for iD easily available and the UI handles the case quite OK:
Migration info:
Which then shows as "unspecified" in the UI, which I think is OK:
I will merge this now in order to get it going and maybe improve upon it later.
Description, Motivation & Context
There was already a PR #918 about this. I'm willing to finish it now.
Related issues
Closes #879
Links and data
Relevant OSM Wiki links:
Relevant tag usage stats:
Checklist and Test-Documentation Template
Read on to get your PR merged faster…
Follow these steps to test your PR yourself and make it a lot easier and faster for maintainers to check and approve it.
This is how it works:
After you submit your PR, the system will create a preview and comment on your PR:
Once the preview is ready, use it to test your changes.
Now copy the snippet below into a new comment and fill out the blanks.
Now your PR is ready to be reviewed.