-
Notifications
You must be signed in to change notification settings - Fork 558
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
Core, some worlds: Rename sweep_for_events to sweep_for_advancements #3571
base: main
Are you sure you want to change the base?
Conversation
53d4244
to
ab87093
Compare
@Alchav (Can't request review) |
no deprecated |
My bad, I had that, and then I accidentally removed it again lol |
Approval from Espeon is implicit as per https://discord.com/channels/731205301247803413/1214608557077700720/1253206955879694336 |
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 confirm that this was super confusing when first getting into AP. Good change 👍
What, do you really need my approval to rename a core function just because I use it? |
That is the current procedure, yes - If something touches a world, the world maintainer has to approve. We have discussed changing this in the (recent) past, some ppl argued that we shouldn't need to do this, but some world maintainers have said they want to continue to be required to sign off on anything touching their world. Happy to discuss solutions in the Discord. |
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.
1: Approved, that seems like a good thing
2: Why was I requested to review this?
3: Note for the future, if something requires approval from Stardew, even though technically it's my name on it, I consider if Jouramie approves it, it's good enough, as he wrote most of that world with me and is arguably even more qualified than me to approve things related to it
Seems like we're going with the "advancement" naming now, and I have recent evidence that people think this function is referring to
address = None
. So let's try to get away from the old "event" naming and be consistent about "event" meaningaddress = None
and "advancement" meaning "will be picked up by sweep_for_events / etc."I wasn't sure about the
event
parameter oncollect
. I feel like that one might be used slightly differently? Lmk what you think about that one.