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

remove not only $key, but also source:$key and check_date:$key #6067

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mnalis
Copy link
Member

@mnalis mnalis commented Dec 31, 2024

when trimming keys in KEYS_THAT_SHOULD_BE_REMOVED_WHEN_PLACE_IS_REPLACED, remove not only every $key that matches, but also every source:$key and check_date:$key that matches too.

Rationale being that if we are removing no longer relevant key, we should remove its metadata too (e.g. from where it initially came, and when it was last verified).

Tested with debug apk on e.g. https://www.openstreetmap.org/node/832042729 which has source:opening_hours and which regular SC does not remove...

(it also adds --info to gradlew test invocation in GitHub action, to report reason in case of test failure - which I needed and think is useful all around when we run tests)

Copy link
Member

@matkoniecz matkoniecz left a comment

Choose a reason for hiding this comment

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

Can you add test or two?

I would test that:

  • lone source:opening_hours is removed
  • when source:opening_hours and opening_hours are there both are removed

(I can also add them if you do not want, let me know in such case)

@westnordost
Copy link
Member

Hmm, what about Things, then?

I was thinking that maybe it would be a better solution if this is done for any edit? (I.e. if key is edited, source:key is deleted?)

@mnalis mnalis marked this pull request as draft January 2, 2025 00:33
@mnalis
Copy link
Member Author

mnalis commented Jan 2, 2025

I was thinking that maybe it would be a better solution if this is done for any edit?

Agreed! Here is (debug apk that does it that way (in fun remove()) if others want to try it out. I've quickly checked manually on https://www.openstreetmap.org/node/832042729 and it seems to work.

Can you add test or two?

Good idea, will try

@FloEdelmann FloEdelmann changed the title remove not only ${key}, but also source:${key} remove not only $key, but also source:$key and check_date:$key Jan 3, 2025
@mnalis mnalis force-pushed the remove-source-key branch from 0164794 to e22e3b6 Compare January 3, 2025 23:39
@mnalis
Copy link
Member Author

mnalis commented Jan 3, 2025

Current revision of this PR seems to be successful in additionally removing source:$key when $key is removed (And added test seems to confirm it), so in its current state it would fix specific problem reported in #6057.


However, I think it would be good idea to also remove check_date:$key and other related date keys, as those are also obsolete when $key is removed.

Unfortunately trying to do that breaks various tests (see FIXMEs in this PR) as it seems to be removing check_date:$key in cases when modification was done (i.e. when check date should be updated instead of removed).

I've been trying to look into it, but it is at about the boundary of my abilities (or beyond it 😅). I'll give it another try after several days (going AFK now), so:

  • if anyone in the meantime cares to gives me a hint or two where to look, it would be greatly appreciated. Or,
  • if anyone more skilled want to take it from here instead, I'm good with that too. Or,
  • otherwise, this PR could also be merged as-is, as it fixes reported source:$key issue successfully, and fixing other related meta-keys (check_date:$key) and friends) can be left as an exercise for another time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants