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

Simplify featureDictionaryFuture interface in quests #5219

Closed
FloEdelmann opened this issue Aug 28, 2023 · 7 comments · Fixed by #5273
Closed

Simplify featureDictionaryFuture interface in quests #5219

FloEdelmann opened this issue Aug 28, 2023 · 7 comments · Fixed by #5273
Assignees
Labels
code cleanup help wanted help by contributors is appreciated; might be a good first contribution for first-timers

Comments

@FloEdelmann
Copy link
Member

Currently, quests that want to use the feature dictionary, accept a FutureTask<FeatureDictionary>, e.g.

11 to AddAmenityCover(featureDictionaryFuture),

class AddAmenityCover (
private val featureDictionaryFuture: FutureTask<FeatureDictionary>
) : OsmElementQuestType<Boolean> {

As suggested in #5212 (comment) by @westnordost, this interface could be simplified to something like:

class MyQuest(private val getFeature: (tags: Map<String, String>) -> Feature?) { ... }

@qugebert Would you fancy giving it a try?

@FloEdelmann FloEdelmann added help wanted help by contributors is appreciated; might be a good first contribution for first-timers code cleanup labels Aug 28, 2023
@westnordost
Copy link
Member

@FloEdelmann there are lots of small things that could be slightly improved. It doesn't make sense to open issue tickets for all of those. A PR is welcome anyway.

@westnordost
Copy link
Member

westnordost commented Aug 28, 2023

Regarding the feature dictionary, it's worth looking what other uses of the feature dictionary there are and whether it makes sense to simplify these, too.

(Did you pin the iOS ticket #1892 by mistake?)

@FloEdelmann
Copy link
Member Author

FloEdelmann commented Aug 28, 2023

It doesn't make sense to open issue tickets for all of those.

Okay, I just thought I'd open an issue so that we don't forget it. Should we maybe open a collection issue with many of those small tasks (maybe also linking to other issues) instead?

Did you pin the iOS ticket by mistake?

No, I pinned it since it is a highly upvoted issue and contains many actionable tasks, to highlight it for contributors.

@westnordost
Copy link
Member

No, I pinned it since it is a highly upvoted issue and contains many actionable tasks, to highlight it for contributors.

Hm well, any task that has some kind of label in this issue tracker should be actionable. So, I'd say as far as you are concerned about that people who want to contribute to the project but don't know what to do, you don't need to worry, as there is enough of it in the issue tracker. That's my view.

Regarding the tasks in the iOS ticket - well I would say, for most tasks I listed there, most of them do not have an additional benefit other than bringing us one step closer to an eventual iOS port. For this reason, I am not sure if it is worth funneling volunteer time into that as long as there is not a big "OK, go!", i.e. someone or an organization leading the charge.

@qugebert
Copy link
Contributor

qugebert commented Sep 1, 2023

@FloEdelmann
Ok, i can try this but i haven't really understood what i shall do.

Another class that implements all the FeatureDictionary element filtering stuff (Like FeatureYesNoQuest : SimpleYesNoQuest) and AddAmenityCover etc. uses this instead of SimpleYesNoQuest, so we only need to define the quest-specific parameters in AddAmenityCover,AddIsAmentyIndoor,etc. But we still need to pass featureDictionaryFuture in QuestModule?

Or should it work without passing featureDictionaryFuture as a parameter? But i have no idea yet how to do this, so it would need some time.

@westnordost
Copy link
Member

Ok, i can try this but i haven't really understood what i shall do.

Well, don't mind this then.

@qugebert
Copy link
Contributor

qugebert commented Sep 1, 2023

Ok, i can try this but i haven't really understood what i shall do.

Well, don't mind this then.

Ok, I just didn't want to leave it unanswered, since I was addressed directly.

@westnordost westnordost moved this to Done in iOS Port Dec 20, 2023
@westnordost westnordost moved this from Done to Released in iOS Port Dec 20, 2023
@FloEdelmann FloEdelmann self-assigned this Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup help wanted help by contributors is appreciated; might be a good first contribution for first-timers
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

3 participants