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

allow multiple prerequisiteTags #87

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

k-yle
Copy link
Collaborator

@k-yle k-yle commented Feb 26, 2023

Closes #137

Relates to openstreetmap/iD#9511. This is not a breaking change if that PR is merged first. See openstreetmap/iD#9511 for details.

@tordans

This comment was marked as resolved.

@k-yle
Copy link
Collaborator Author

k-yle commented Feb 26, 2023

thanks @tordans, I've updated the README

@tyrasd tyrasd added this to the v7 milestone Mar 3, 2023
@tyrasd tyrasd added breaking and removed breaking labels Mar 3, 2023
README.md Outdated Show resolved Hide resolved
@tordans
Copy link
Collaborator

tordans commented May 25, 2024

I am wondering about two things…

a. I don't think its great to have a key value that can hold an array. It should really be values instead.

b. The note on breaking changes is true for iD, but it is not true for GoMap, EveryDoor and other data consumers of the tagging schema repo.
What would be a good migration path? The easiest would be to make this a major release and explain the update path for the other apps.

I don't see any way to do a soft migration with deprecation and both solutions being present for one release cycle. That would be ideal to release fast but allow consumer apps to update afterwards.

@k-yle
Copy link
Collaborator Author

k-yle commented May 25, 2024

a. I don't think its great to have a key value that can hold an array. It should really be values instead.

Are you suggesting that we allow both value XOR values? Or should value be replaced by values?

b. The note on breaking changes is true for iD, but it is not true for GoMap, EveryDoor and other data consumers of the tagging schema repo.

Based on 544f546, it looks like there's about to be a major release, so we may as well label this as breaking

It would be worth prewarning GoMap and EveryDoor, since they're both fetching directly from the main branch...12 Ideally all data consumers would be setup to only fetch v6.x.x, for example, using https://unpkg.com/@openstreetmap/id-tagging-schema@6/dist

Footnotes

  1. https://github.com/bryceco/GoMap/blob/c5795d1/src/presets/update.sh#L3

  2. https://github.com/Zverik/every_door/blob/a00a2c6/tools/json_to_sqlite.py#L120

@tordans
Copy link
Collaborator

tordans commented May 26, 2024

Are you suggesting that we allow both value XOR values? Or should value be replaced by values?

I think the best would be to make it a breaking change and migrate everything to values. Maybe the builder should have a fallback to listen for value as well (and make it an array) so we don't have issues with updating all the code at once over at https://github.com/openstreetmap/id-tagging-schema

…so we may as well label this as breaking

Sounds like the cleanest solution to me.

It would be worth prewarning GoMap and EveryDoor

Agreed. Lets wait for Martin and then create an issue to ping the known data users.

schemas/field.json Outdated Show resolved Hide resolved
schemas/field.json Outdated Show resolved Hide resolved
@tordans
Copy link
Collaborator

tordans commented May 31, 2024

On the question of a mayor release and how to inform consumers of the data:

@tyrasd just told me he looked into this with a breaking change in 2022 (eg. here, here). Most editors should be using a specific major release. GoMap does not, but that is fine because Bryce pulls the change manually via script so he is informed of breaking changes this way.

@k-yle k-yle added the breaking label Jun 1, 2024
@k-yle k-yle force-pushed the multiple-prerequisiteTag branch from 75d757e to a6413bb Compare June 1, 2024 12:35
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.

allow more complex prerequisiteTags for fields
3 participants