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

Define achievements inside quest types #3327

Merged
merged 13 commits into from
Oct 1, 2021
Merged

Define achievements inside quest types #3327

merged 13 commits into from
Oct 1, 2021

Conversation

FloEdelmann
Copy link
Member

As suggested in #3311 (comment):

Maybe the achievement stuff should be refactored so that it is defined in the quests themselves to which achievements they should count. Contributors keep forgetting to add it

@FloEdelmann FloEdelmann marked this pull request as draft September 28, 2021 07:43
Copy link
Member Author

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Unfortunately, I got stuck in one place (isAchieved function in AchievementGiver class, see TODO comment added in last commit).

@westnordost Could you please already comment on the general direction of this PR and maybe guide me how to fix the issue that is now blocking me?

@FloEdelmann FloEdelmann marked this pull request as ready for review September 28, 2021 17:55
Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

I stumbled upon the .has, thought at first it is some extension function from Kotlin alike .contains. Maybe it would be better to write it out, it's not much longer.

But in any case, otherwise, everything looks good! Thank you!

@smichel17 smichel17 mentioned this pull request Oct 1, 2021
@westnordost westnordost merged commit 2763138 into streetcomplete:master Oct 1, 2021
@FloEdelmann FloEdelmann deleted the achievements branch October 2, 2021 05: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.

None yet

3 participants